#27253 closed defect (fixed)
networkx fails to build with SAGE_CHECK="yes"
Reported by: | Konrad127123 | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-8.7 |
Component: | packages: standard | Keywords: | networkx, nose, build |
Cc: | Merged in: | ||
Authors: | Konrad K. Dabrowski | Reviewers: | Dima Pasechnik |
Report Upstream: | N/A | Work issues: | |
Branch: | e1ce380 (Commits, GitHub, GitLab) | Commit: | |
Dependencies: | Stopgaps: |
Description
After #26326, networkx
fails to build with SAGE_CHECK="yes"
unless the optional package nose
is installed.
[networkx-2.2] Running the test suite for networkx-2.2... [networkx-2.2] Testing networkx requires the package nose to be installed [networkx-2.2] [networkx-2.2] real 0m0.004s [networkx-2.2] user 0m0.004s [networkx-2.2] sys 0m0.001s [networkx-2.2] ************************************************************************ [networkx-2.2] Error testing package networkx-2.2 [networkx-2.2] ************************************************************************
Change History (17)
comment:1 Changed 3 years ago by
- Branch set to u/Konrad127123/networkx_fails_to_build_with_sage_check__yes_
comment:2 Changed 3 years ago by
- Commit set to e1ce380572fe82b6238f920299a614447f36843c
- Status changed from new to needs_review
comment:3 Changed 3 years ago by
Can you dig up the info on nose
's license. Cause it needs to be GPL-compatible to be standard.
comment:4 Changed 3 years ago by
OK, it's fine. See https://github.com/nose-devs/nose/blob/master/lgpl.txt Could you please update the nose's SPKG.txt file with this info?
comment:5 Changed 3 years ago by
nose's SPKG.txt already contains
== License == GNU LGPL
comment:6 Changed 3 years ago by
- Reviewers set to Dima Pasechnik
- Status changed from needs_review to positive_review
OK, sorry, I was obviously blind :-)
comment:7 Changed 3 years ago by
- Branch changed from u/Konrad127123/networkx_fails_to_build_with_sage_check__yes_ to e1ce380572fe82b6238f920299a614447f36843c
- Resolution set to fixed
- Status changed from positive_review to closed
comment:8 Changed 3 years ago by
- Commit e1ce380572fe82b6238f920299a614447f36843c deleted
Guys, test harnesses are not automatically standard packages. Instead the spkg-check script should just abort if nose is not found.
comment:9 Changed 3 years ago by
So is nose
going to be standard (bypassing the required discussion and vote) or is this change going to be reverted?
comment:10 Changed 3 years ago by
I think this falls under a situation of a new requirement by a standard package. We have been adding various Python deps like this quite easily...
comment:11 Changed 3 years ago by
I view Volker's comment as disagreeing with you. Could you respond to that, please?
comment:12 follow-up: ↓ 13 Changed 3 years ago by
Building a standard package with SAGE_CHECK=yes must work. If anyone can offer a different solution to this, without making nose
standard, fine...
But please do not offer removing checks just cause you do not want to promote nose
...
comment:13 in reply to: ↑ 12 Changed 3 years ago by
Replying to dimpase:
Building a standard package with SAGE_CHECK=yes must work. If anyone can offer a different solution to this, without making
nose
standard, fine...
Volker did that.
But please do not offer removing checks just cause you do not want to promote
nose
...
But you don't like his solution, so you're discarding it.
The use of nose
was added at #26326, and that ticket description says quite clearly, "Note that self tests require nose
which is optional only." So it's not like this situation is a surprise or was unintended when the self-tests were implemented. I would deduce that the intention was: if nose
is present, use it, otherwise skip the self tests. Or maybe the intention was: most people don't run the self tests, and those that do can install nose
. In any case, I don't agree that #26326 forces everyone to install nose
.
comment:14 Changed 3 years ago by
A Dieselgate-style "solution" is not a solution. This is already merged, and let us not start track ticket wars caused by a 300K addition to Sage tarballs.
comment:15 follow-up: ↓ 16 Changed 3 years ago by
ISTM running the tests for networkx also requires scipy, so perhaps scipy should be a prerequisite dependency.
comment:16 in reply to: ↑ 15 Changed 3 years ago by
Replying to embray:
ISTM running the tests for networkx also requires scipy, so perhaps scipy should be a prerequisite dependency.
yes, this is a good catch. Should we do a quick gitlab fix?
comment:17 Changed 3 years ago by
Followup in #27515
Suggested fix: add
nose
as a runtime dependency ofnetworkx
. This meansnose
needs to be a standard package.New commits:
networkx needs a runtime dependency on nose, otherwise it fails to build with SAGE_CHECK="yes". This means nose has to be upgraded to a standard package.