Nir Soffer has uploaded a new change for review.
Change subject: build: Disable tests during build ......................................................................
build: Disable tests during build
Tests are needed for development, not for building a package. This allows us to use latest and greatest development tools, which are not available in brew or koji.
Since we install nose using pip, remove the build requires - we don't need now nose to build rpms.
Change-Id: I9e3589c365166f934f117b53c65cea4b90db3516 Signed-off-by: Nir Soffer nsoffer@redhat.com --- M vdsm.spec.in 1 file changed, 0 insertions(+), 6 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/66/63966/1
diff --git a/vdsm.spec.in b/vdsm.spec.in index f234b09..c7f6dde 100644 --- a/vdsm.spec.in +++ b/vdsm.spec.in @@ -62,7 +62,6 @@ BuildRequires: python2-devel BuildRequires: python-mock BuildRequires: python-netaddr -BuildRequires: python-nose BuildRequires: python-six >= 1.9.0 BuildRequires: rpm-build
@@ -95,10 +94,8 @@
%if 0%{?with_python3} %if 0%{?rhel} -BuildRequires: python34-nose BuildRequires: python34-six %else # fedora -BuildRequires: python3-nose BuildRequires: python3-six BuildRequires: python3-netaddr BuildRequires: python3-yaml @@ -781,9 +778,6 @@ # Install the libvirt hook for cleaning up the XML install -Dm 0755 vdsm/virt/libvirt-hook.sh \ %{buildroot}%{_sysconfdir}/libvirt/hooks/qemu - -%check -make tests
%pre # Force standard locale behavior (English)
gerrit-hooks has posted comments on this change.
Change subject: build: Disable tests during build ......................................................................
Patch Set 1:
* Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
gerrit-hooks has posted comments on this change.
Change subject: build: Disable tests during build ......................................................................
Patch Set 2:
* Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
gerrit-hooks has posted comments on this change.
Change subject: build: Disable tests during build ......................................................................
Patch Set 3:
* Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Edward Haas has posted comments on this change.
Change subject: build: Disable tests during build ......................................................................
Patch Set 3: Code-Review-1
Now you are missing nose for python3. You either need to leave the python3 nose alone in check-* or install pip3 with nose. -1 for visibility.
Dan Kenigsberg has posted comments on this change.
Change subject: build: Disable tests during build ......................................................................
Patch Set 3:
(1 comment)
https://gerrit.ovirt.org/#/c/63966/3//COMMIT_MSG Commit Message:
Line 5: CommitDate: 2016-09-15 01:45:00 +0300 Line 6: Line 7: build: Disable tests during build Line 8: Line 9: Tests are needed for development, not for building a package. This I don't subscribe to this postulate. spec files have a %check section for a reason: to improve the chances that a sane code is being built. Line 10: allows us to use latest and greatest development tools, which are not Line 11: available in brew or koji. Line 12: Line 13: Since we install nose using pip, remove the build requires - we don't
Nir Soffer has posted comments on this change.
Change subject: build: Disable tests during build ......................................................................
Patch Set 3:
(1 comment)
https://gerrit.ovirt.org/#/c/63966/3//COMMIT_MSG Commit Message:
Line 5: CommitDate: 2016-09-15 01:45:00 +0300 Line 6: Line 7: build: Disable tests during build Line 8: Line 9: Tests are needed for development, not for building a package. This
I don't subscribe to this postulate. spec files have a %check section for a
This means you are subscribes to using old development tools, and have to fix the nose check regression :-) Line 10: allows us to use latest and greatest development tools, which are not Line 11: available in brew or koji. Line 12: Line 13: Since we install nose using pip, remove the build requires - we don't
Dan Kenigsberg has posted comments on this change.
Change subject: build: Disable tests during build ......................................................................
Patch Set 3:
(1 comment)
https://gerrit.ovirt.org/#/c/63966/3//COMMIT_MSG Commit Message:
Line 5: CommitDate: 2016-09-15 01:45:00 +0300 Line 6: Line 7: build: Disable tests during build Line 8: Line 9: Tests are needed for development, not for building a package. This
This means you are subscribes to using old development tools, and have to f
or skip the test on "broken" systems with buggy nose, until nose is fixed. Line 10: allows us to use latest and greatest development tools, which are not Line 11: available in brew or koji. Line 12: Line 13: Since we install nose using pip, remove the build requires - we don't
Edward Haas has posted comments on this change.
Change subject: build: Disable tests during build ......................................................................
Patch Set 3:
(1 comment)
https://gerrit.ovirt.org/#/c/63966/3//COMMIT_MSG Commit Message:
Line 5: CommitDate: 2016-09-15 01:45:00 +0300 Line 6: Line 7: build: Disable tests during build Line 8: Line 9: Tests are needed for development, not for building a package. This
or skip the test on "broken" systems with buggy nose, until nose is fixed.
Sounds like the 'skip' pattern. Being consistent is important, tests should either run or not, either pass or fail. Mixing is not helpful.
I do not see the benefit of testing as part of creating rpms. This is a flow that makes sense to me: [develop]-->[run tests]-->[compile]-->[run all tests]-->[commit]
After that is over, we assume the commits on the branch are stable and fully tested. At that point we go and build the deployment packages. There is no reason to run unit tests at that point, we MUST assume they are ok, otherwise something in the flow is broken. Line 10: allows us to use latest and greatest development tools, which are not Line 11: available in brew or koji. Line 12: Line 13: Since we install nose using pip, remove the build requires - we don't
Piotr Kliczewski has posted comments on this change.
Change subject: build: Disable tests during build ......................................................................
Patch Set 3:
Developers make sure that the flow is OK but we all are humans and we cut corners or forget sometimes. In my opinion there is no harm in run the tests one more time.
We can make a lot of assumptions but we will see that there will be situations when they do not hold.
We could add conditional skip if tests are failing due to bugs in specific distributions and make sure that there are marked properly so when nose is fixed we can enable them again.
Yaniv Bronhaim has posted comments on this change.
Change subject: build: Disable tests during build ......................................................................
Patch Set 3:
most developers run "make rpm" directly instead of "make check", as this is what we advice to do in [1]. with this patch developers will probably avoid running the tests at all
[1]https://www.ovirt.org/develop/developer-guide/vdsm/developers/
Yaniv Bronhaim has posted comments on this change.
Change subject: build: Disable tests during build ......................................................................
Patch Set 3: Code-Review+1
Maor Lipchuk has posted comments on this change.
Change subject: build: Disable tests during build ......................................................................
Patch Set 3: Verified+1
My VDSM failed with NOSE version even with commit 4c0a7e27945c82ce043a872ceac8a509bc5676c4 which supposed to fix that.
Cherry picked this test and verified
Maor Lipchuk has posted comments on this change.
Change subject: build: Disable tests during build ......................................................................
Patch Set 3:
/s/test/patch
gerrit-hooks has posted comments on this change.
Change subject: build: Disable tests during build ......................................................................
Patch Set 4:
* Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Nir Soffer has posted comments on this change.
Change subject: build: Disable tests during build ......................................................................
Patch Set 4:
This version leaves python3-nose as is.
Nir Soffer has posted comments on this change.
Change subject: build: Disable tests during build ......................................................................
Patch Set 4:
See also https://github.com/oVirt/ovirt-site/pull/516, updating developer documentation.
Nir Soffer has posted comments on this change.
Change subject: build: Disable tests during build ......................................................................
Patch Set 4: Verified+1
Edward Haas has posted comments on this change.
Change subject: build: Disable tests during build ......................................................................
Patch Set 4:
(2 comments)
https://gerrit.ovirt.org/#/c/63966/4/configure.ac File configure.ac:
Line 298 Line 299 Line 300 Line 301 Line 302 If we have not touched nose for py3, why not leaving this check in?
Line 307 Line 308 Line 309 Line 310 Line 311 I guess the check should now move the the makefile. This check is now covered by the nose minimum version validation?
Nir Soffer has posted comments on this change.
Change subject: build: Disable tests during build ......................................................................
Patch Set 4:
(2 comments)
https://gerrit.ovirt.org/#/c/63966/4/configure.ac File configure.ac:
Line 298 Line 299 Line 300 Line 301 Line 302
If we have not touched nose for py3, why not leaving this check in?
Right, I'll leave this line as is.
Line 307 Line 308 Line 309 Line 310 Line 311
I guess the check should now move the the makefile.
We cannot use this check since nose is not installed at this point, it is installed later during make check.
gerrit-hooks has posted comments on this change.
Change subject: build: Disable tests during build ......................................................................
Patch Set 5:
* Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Nir Soffer has posted comments on this change.
Change subject: build: Disable tests during build ......................................................................
Patch Set 4:
(1 comment)
https://gerrit.ovirt.org/#/c/63966/4/configure.ac File configure.ac:
Line 298 Line 299 Line 300 Line 301 Line 302
Right, I'll leave this line as is.
Fixed in current version.
Nir Soffer has posted comments on this change.
Change subject: build: Disable tests during build ......................................................................
Patch Set 5: Verified+1
Edward Haas has posted comments on this change.
Change subject: build: Disable tests during build ......................................................................
Patch Set 5: Code-Review+1
(1 comment)
https://gerrit.ovirt.org/#/c/63966/4/configure.ac File configure.ac:
Line 307 Line 308 Line 309 Line 310 Line 311
We cannot use this check since nose is not installed at this point, it is i
Right, but don't we want to check it in an early stage of the test anyway? I guess it does not matter much, it would just be a nice to have: explicitly warn that nose is missing if a test run is attempted.
Nir Soffer has posted comments on this change.
Change subject: build: Disable tests during build ......................................................................
Patch Set 4:
(1 comment)
https://gerrit.ovirt.org/#/c/63966/4/configure.ac File configure.ac:
Line 307 Line 308 Line 309 Line 310 Line 311
Right, but don't we want to check it in an early stage of the test anyway?
We cannot check in configure since configure is run during make rpm.
We have a check in make tests - if you run "make check" or "make tests" and you don't have nose or have an older version, your build will fail.
gerrit-hooks has posted comments on this change.
Change subject: build: Disable tests during build ......................................................................
Patch Set 6:
* Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Dan Kenigsberg has posted comments on this change.
Change subject: build: Disable tests during build ......................................................................
Patch Set 6: Code-Review-1
(1 comment)
https://gerrit.ovirt.org/#/c/63966/6/vdsm.spec.in File vdsm.spec.in:
Line 65 Line 66 Line 67 Line 68 Line 69 if we are not running the test, we do not need to pull the following packages. As the comment says, they are required only for test, not for build per se.
Nir Soffer has posted comments on this change.
Change subject: build: Disable tests during build ......................................................................
Patch Set 6:
(1 comment)
https://gerrit.ovirt.org/#/c/63966/6/vdsm.spec.in File vdsm.spec.in:
Line 65 Line 66 Line 67 Line 68 Line 69
if we are not running the test, we do not need to pull the following packag
Right, will remove in the next version.
gerrit-hooks has posted comments on this change.
Change subject: build: Disable tests during build ......................................................................
Patch Set 7:
* Update Tracker::IGNORE, no bug url/s found * Check Bug-Url::IGNORE, not relevant for branch: master * Check Public Bug::WARN, no public bug url found * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Nir Soffer has posted comments on this change.
Change subject: build: Disable tests during build ......................................................................
Patch Set 6:
(1 comment)
https://gerrit.ovirt.org/#/c/63966/6/vdsm.spec.in File vdsm.spec.in:
Line 65 Line 66 Line 67 Line 68 Line 69
Right, will remove in the next version.
The unneeded packages are remove in the next patch.
Nir Soffer has posted comments on this change.
Change subject: build: Disable tests during build ......................................................................
Patch Set 7:
Dan, the vdsm developers page is already updated for the new build, so we better merge this soon.
See http://www.ovirt.org/develop/developer-guide/vdsm/developers/
From Dan Kenigsberg danken@redhat.com:
Dan Kenigsberg has posted comments on this change.
Change subject: build: Disable tests during build ......................................................................
Patch Set 7: Code-Review+2
From Dan Kenigsberg danken@redhat.com:
Dan Kenigsberg has submitted this change and it was merged.
Change subject: build: Disable tests during build ......................................................................
build: Disable tests during build
Tests are needed for development, not for building a package. This allows us to use latest and greatest development tools, which are not available in brew or koji.
Since we install nose using pip, remove the build requires - we don't need now nose to build rpms.
Also remove nose from the automation *.packages files, since we install it from pip.
Finally remove the nose check from configure, since configure is run during the build, and we cannot require nose at this point. We are checking nose when running the tests.
We still install python 3 nose using the distribution packages, since it is new enough, and mixing pip and pip3 on same machine does not work well.
Change-Id: I9e3589c365166f934f117b53c65cea4b90db3516 Signed-off-by: Nir Soffer nsoffer@redhat.com Reviewed-on: https://gerrit.ovirt.org/63966 Reviewed-by: Edward Haas edwardh@redhat.com Continuous-Integration: Jenkins CI Reviewed-by: Dan Kenigsberg danken@redhat.com --- M automation/check-merged.packages.el7 M automation/check-merged.packages.fc24 M automation/check-patch.packages.el7 M automation/check-patch.packages.fc24 M configure.ac M vdsm.spec.in 6 files changed, 0 insertions(+), 14 deletions(-)
Approvals: Nir Soffer: Verified Jenkins CI: Passed CI tests Dan Kenigsberg: Looks good to me, approved Edward Haas: Looks good to me, but someone else must approve
vdsm-patches@lists.fedorahosted.org
-
automation＠ovirt.org -
Code Review -
Dan Kenigsberg -
edwardh＠redhat.com -
Maor Lipchuk -
Nir Soffer -
Piotr Kliczewski -
ybronhei＠redhat.com