Opened 9 months ago

Last modified 3 months ago

#32913 needs_review defect

Revert #32713 (Apply "configure --enable-editable" to other packages) for sage-conf, sage-setup

Reported by: mkoeppe Owned by:
Priority: critical Milestone: sage-duplicate/invalid/wontfix
Component: build Keywords:
Cc: gh-tobiasdiez, klee Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: public/build/trac_editable (Commits, GitHub, GitLab) Commit: 2a302097ca6764d8305667f93f074609b76a32f1
Dependencies: Stopgaps:

Status badges

Description (last modified by mkoeppe)

(from https://trac.sagemath.org/ticket/32713#comment:11)

Users need the wheel of sage-conf when creating venvs. And we need the wheels of packages sage-conf and sage-setup when testing distributions using ./sage -sh -c '(cd pkgs/sagemath-standard && tox -v -v -v -e python-sagewheels-nopypi)' and similar - as documented in the developer's guide as of #32899.

Until editable wheels (PEP 660) are implemented in setuptools (or we switch the distributions to another build system that implements PEP 660 such as flit), we revert the change made in #32713 for these packages.

But we keep sage-docbuild editable.

Change History (22)

comment:1 Changed 9 months ago by mkoeppe

  • Description modified (diff)

comment:2 Changed 9 months ago by mkoeppe

  • Branch set to u/mkoeppe/revert__32713__apply__configure___enable_editable__to_other_packages__for_sage_conf__sage_setup

comment:3 Changed 9 months ago by mkoeppe

  • Commit set to 2cc04976e540af5452913d34948c9a74d088bff5
  • Status changed from new to needs_review

New commits:

2cc0497build/pkgs/{sage_conf,sage_setup}/install: Do not use sdh_pip_editable_install

comment:4 follow-ups: Changed 9 months ago by gh-tobiasdiez

Can you please expand on why this leads to problems? For example, end-users should normally not use an editable install anyway, neither should the build process for the wheels on ci. For the tox cmd, wouldn't be better if the tox config of sagemath-standard only applies the -e only to that package if that's the desired behavior.

I would fine it confusing if make --enable-editable-install installs some packages in editable mode and others not.

comment:5 in reply to: ↑ 4 Changed 9 months ago by mkoeppe

Replying to gh-tobiasdiez:

For the tox cmd, wouldn't be better if the tox config of sagemath-standard only applies the -e only to that package if that's the desired behavior.

Please take a look at pkgs/sagemath-standard/tox.ini, then you'll see what it does...

comment:6 in reply to: ↑ 4 Changed 9 months ago by mkoeppe

Replying to gh-tobiasdiez:

I would fine it confusing if configure --enable-editable installs some packages in editable mode and others not.

Hasn't confused anyone so far...

comment:7 Changed 9 months ago by mkoeppe

Also note that configure --help explains:

  --enable-editable       use an editable install of the Sage library

comment:8 Changed 9 months ago by mkoeppe

  • Cc klee added
  • Description modified (diff)

comment:9 Changed 8 months ago by gh-tobiasdiez

Sorry, but I still don't understand why you need to build and use wheels for sage packages if all of them use editable installs. Why can one not install them (as editable) in the tox environments?

For example, something along the following lines (in src/tox):

[testenv]
usedevelop=True # Instruct tox to use editable install of main sagemath package
deps=
  --editable=file:///{toxinidir}/../path/to/sage_conf # Install sage-conf as editable install

Potentially combined with a custom install_command that invokes pip with find-links to discover the wheels of other dependencies that are build during make.

comment:10 follow-up: Changed 8 months ago by mkoeppe

That could be implemented but it is not implemented.

comment:11 Changed 8 months ago by mkoeppe

And the installation procedure would also be more difficult to document than just to say "install all wheels from ...".

comment:12 Changed 8 months ago by mkoeppe

#32616 may be a way to resolve this

comment:13 in reply to: ↑ 10 Changed 8 months ago by gh-tobiasdiez

Replying to mkoeppe:

That could be implemented but it is not implemented.

I would strongly prefer that over reverting something that works in principle. I'll have a look the coming days and create a prototype.

comment:14 Changed 8 months ago by mkoeppe

Any progress on this?

comment:15 Changed 8 months ago by gh-tobiasdiez

  • Branch changed from u/mkoeppe/revert__32713__apply__configure___enable_editable__to_other_packages__for_sage_conf__sage_setup to public/build/trac_editable
  • Commit changed from 2cc04976e540af5452913d34948c9a74d088bff5 to 2a302097ca6764d8305667f93f074609b76a32f1
  • Status changed from needs_review to needs_work

With the new branch public/build/trac_editable you can run tox -e py38 (or 39) from the src folder and get a tox environment with editable installs of sagemath-standard and sage-conf. Some of the tests are failing (because gap and blas are not using sage-conf but determine stuff at runtime during env variables) and there might be a more elegant way to set the env variables but as a prototype this should suffice.


New commits:

2a30209Add tox env with editable installs

comment:16 Changed 8 months ago by mkoeppe

This branch doesn't fix what is described in the ticket description

comment:17 Changed 8 months ago by gh-tobiasdiez

Maybe not (as I said above in comment 4 I don't quite get what your problem is). But it shows how to use editable installs with tox and thus provides a way around the problem "we need the wheels of packages sage-conf and sage-setup when testing distributions [with tox]". Also it was meant as a prototype not as a complete solution.

comment:18 Changed 8 months ago by mkoeppe

Well we are in the release phase for Sage 9.5 and can't merge "prototypes"

comment:19 Changed 6 months ago by slelievre

  • Milestone changed from sage-9.5 to sage-9.6

Set milestone to sage-9.6 after Sage 9.5 release.

comment:20 Changed 3 months ago by mkoeppe

  • Milestone changed from sage-9.6 to sage-9.7

comment:21 Changed 3 months ago by mkoeppe

  • Milestone changed from sage-9.7 to sage-duplicate/invalid/wontfix

I have a different solution in #33817

comment:22 Changed 3 months ago by mkoeppe

  • Authors Matthias Koeppe deleted
  • Status changed from needs_work to needs_review
Note: See TracTickets for help on using tickets.