Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#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:

Status badges

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 Konrad127123

  • Branch set to u/Konrad127123/networkx_fails_to_build_with_sage_check__yes_

comment:2 Changed 3 years ago by Konrad127123

  • Authors set to Konrad K. Dabrowski
  • Commit set to e1ce380572fe82b6238f920299a614447f36843c
  • Status changed from new to needs_review

Suggested fix: add nose as a runtime dependency of networkx. This means nose needs to be a standard package.


New commits:

e1ce380networkx 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.

comment:3 Changed 3 years ago by dimpase

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 dimpase

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 Konrad127123

nose's SPKG.txt already contains

== License ==

GNU LGPL

comment:6 Changed 3 years ago by dimpase

  • 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 vbraun

  • 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 vbraun

  • 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 jhpalmieri

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 dimpase

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 jhpalmieri

I view Volker's comment as disagreeing with you. Could you respond to that, please?

comment:12 follow-up: Changed 3 years ago by 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...

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 jhpalmieri

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 dimpase

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: Changed 3 years ago by embray

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 dimpase

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 embray

Followup in #27515

Note: See TracTickets for help on using tickets.