Ignore DESTDIR when installing to _skbuild #475
Conversation
jokva
force-pushed
the
ignore-destdir-internal-install
branch
2 times, most recently
from
5613734
to
a8cec5f
Compare
|
Alright, this should be more-or-less complete now. I'm not sure about the failing tests, they shouldn't be affected at all by this patch. |
skbuild/cmaker.py
Outdated
| @@ -491,6 +519,8 @@ def make(self, clargs=(), config="Release", source_dir=".", env=None): | |||
| shlex.split(os.environ.get("SKBUILD_BUILD_OPTIONS", ""))) | |||
| ) | |||
|
|
|||
| env = env_without_destdir(env) | |||
There was a problem hiding this comment.
Instead, you would then update
def make(self, clargs=(), config="Release", source_dir=".", env=None):
to be something like this
@env_without_destdir
def make(self, clargs=(), config="Release", source_dir=".", env=None):
There was a problem hiding this comment.
I don't think this will work out well, at least not without breaking the interface. The env= parameter is forwarded to subprocess.call, which expects either None (inherit parent's environment), or a specific environment as a dict. Unless the context decorator implements dict as well, some translation is needed.
Also, it would have to quietly hijack the env= argument. I ended up writing it as a context manager though, but with an explicit with. One benefit I think is the communication of intent, it's really just for the internal install target the environment variables should be ignored.
The cmake --build --target install command is used to move built artefacts to _skbuild/, but when used in an environment with staged installs (DESTDIR [1] [2]) the _skbuild directory also ends up there. This is never desired as the _skbuild is an internal detail to the scikit-build process, and DESTDIR is not meant to be used to specify the build location. [1] Happens in larger projects where scikit-build is not the only player [2] https://github.com/equinor/segyio/blob/0c791d0f0ae4973058a51e2014837946e668902b/python/setup.py
Running a command in a specific, modified environment, and then restoring the original environment, is useful also outside a testing context. Move the push_env to skbuild.units and update the imports. At the same time, make it derive from the ContextDecorator for some extra bells and whistles.
jokva
force-pushed
the
ignore-destdir-internal-install
branch
from
a8cec5f
to
12a9399
Compare
|
Waiting the CI issues are fixed, I will manually test on windows and then integrate. Thanks for your patience. |
|
Not a problem :---) I've applied the same hack in my downstream problems, which I can revert when this is integrated (and released) here. |
|
Just to chime in that I ran into this and it was very confusing. If this can't be merged, a simple warning shouting to the user that DESTDIR is set would hopefully give them the hint. |
The cmake --build --target install command is used to move built
artefacts to _skbuild/, but when used in an environment with staged
installs (DESTDIR [1] [2]) the _skbuild directory also ends up there.
This is never desired as the _skbuild is an internal detail to the
scikit-build process, and DESTDIR is not meant to be used to specify the
build location.
[1] Happens in larger projects where scikit-build is not the only player
[2] https://github.com/equinor/segyio/blob/0c791d0f0ae4973058a51e2014837946e668902b/python/setup.py