Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
mkoeppe opened this issue Nov 20, 2021 · 24 comments

Comments

@mkoeppe
Copy link
Member

mkoeppe commented Nov 20, 2021

(from #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.

CC: @tobiasdiez @kwankyu

Component: build

Branch/Commit: public/build/trac_editable @ 2a30209

Issue created by migration from https://trac.sagemath.org/ticket/32913

@mkoeppe mkoeppe added this to the sage-9.5 milestone Nov 20, 2021
@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 20, 2021

@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 20, 2021

Commit: 2cc0497

@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 20, 2021

New commits:

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

@tobiasdiez
Copy link
Contributor

comment:4

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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 21, 2021

comment:5

Replying to @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...

@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 21, 2021

comment:6

Replying to @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...

@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 21, 2021

comment:7

Also note that configure --help explains:

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

@mkoeppe

This comment has been minimized.

@tobiasdiez
Copy link
Contributor

comment:9

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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 6, 2021

comment:10

That could be implemented but it is not implemented.

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 6, 2021

comment:11

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

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 6, 2021

comment:12

#32616 may be a way to resolve this

@tobiasdiez
Copy link
Contributor

comment:13

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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 12, 2021

comment:14

Any progress on this?

@tobiasdiez
Copy link
Contributor

comment:15

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

@tobiasdiez
Copy link
Contributor

@tobiasdiez
Copy link
Contributor

Changed commit from 2cc0497 to 2a30209

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 19, 2021

comment:16

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

@tobiasdiez
Copy link
Contributor

comment:17

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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 19, 2021

comment:18

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

@slel
Copy link
Member

slel commented Jan 30, 2022

comment:19

Set milestone to sage-9.6 after Sage 9.5 release.

@slel slel removed this from the sage-9.5 milestone Jan 30, 2022
@slel slel added this to the sage-9.6 milestone Jan 30, 2022
@mkoeppe mkoeppe modified the milestones: sage-9.6, sage-9.7 May 3, 2022
@mkoeppe
Copy link
Member Author

mkoeppe commented May 6, 2022

comment:21

I have a different solution in #33817

@mkoeppe mkoeppe removed this from the sage-9.7 milestone May 6, 2022
@mkoeppe
Copy link
Member Author

mkoeppe commented May 6, 2022

Changed author from Matthias Koeppe to none

@mkoeppe mkoeppe closed this as not planned Won't fix, can't repro, duplicate, stale Feb 8, 2023
mkoeppe added a commit to mkoeppe/sage that referenced this issue Feb 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants