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

Fix: Build environments not activated #4665

Merged
merged 8 commits into from
Dec 5, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions conda_build/build.py
Original file line number Diff line number Diff line change
Expand Up @@ -2584,8 +2584,11 @@ def construct_metadata_for_test(recipedir_or_package, config):


def write_build_scripts(m, script, build_file):
with utils.path_prepended(m.config.host_prefix):
with utils.path_prepended(m.config.build_prefix):
# TODO: Prepending the prefixes here should probably be guarded by
# if not m.activate_build_script:
# Leaving it as is, for now, since we need a quick, non-disruptive patch release.
with utils.path_prepended(m.config.host_prefix, False):
with utils.path_prepended(m.config.build_prefix, False):
Comment on lines +2587 to +2591
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before removing this in a future release, we should add more test cases.
The test added in this PR only checks for the case when there is build+host, activate=True and no outputs.
Other configurations that should be tested:

  • activate=False
  • build and host merged
    • with activate=True
    • with activate=False
  • recipes with outputs
    • and those for potential combinations of the aforementioned other cases.

I consider this out of scope for this PR since we should put out a patch release quickly and because those other cases touch other issues like #3993 -- that's another can of worms (one we should take care of, but not hastily in a small patch release).

env = environ.get_dict(m=m)
env["CONDA_BUILD_STATE"] = "BUILD"

Expand Down
1 change: 0 additions & 1 deletion conda_build/environ.py
Original file line number Diff line number Diff line change
Expand Up @@ -663,7 +663,6 @@ def system_vars(env_dict, m, prefix):
return os_vars(m, prefix)


@lru_cache(maxsize=None)
def os_vars(m, prefix):
d = dict()
# note the dictionary is passed in here - variables are set in that dict if they are non-null
Expand Down
2 changes: 2 additions & 0 deletions conda_build/metadata.py
Original file line number Diff line number Diff line change
Expand Up @@ -1046,6 +1046,8 @@ def stringify_numbers():


class MetaData:
__hash__ = None # declare as non-hashable to avoid its use with memoization

def __init__(self, path, config=None, variant=None):

self.undefined_jinja_vars = []
Expand Down
10 changes: 5 additions & 5 deletions conda_build/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -950,11 +950,10 @@ def get_build_folders(croot):


def prepend_bin_path(env, prefix, prepend_prefix=False):
# bin_dirname takes care of bin on *nix, Scripts on win
env['PATH'] = join(prefix, bin_dirname) + os.pathsep + env['PATH']
env['PATH'] = join(prefix, "bin") + os.pathsep + env['PATH']
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

conda activate always adds "bin" to PATH. We have to have it here too. The addition of "Scripts" is taken care of a few lines below.

if sys.platform == "win32":
env['PATH'] = join(prefix, "Library", "mingw-w64", "bin") + os.pathsep + \
join(prefix, "Library", "usr", "bin") + os.pathsep + os.pathsep + \
join(prefix, "Library", "usr", "bin") + os.pathsep + \
join(prefix, "Library", "bin") + os.pathsep + \
join(prefix, "Scripts") + os.pathsep + \
env['PATH']
Expand Down Expand Up @@ -985,9 +984,10 @@ def sys_path_prepended(prefix):


@contextlib.contextmanager
def path_prepended(prefix):
def path_prepended(prefix, prepend_prefix=True):
# FIXME: Unclear why prepend_prefix=True for all platforms.
old_path = os.environ['PATH']
os.environ['PATH'] = prepend_bin_path(os.environ.copy(), prefix, True)['PATH']
os.environ['PATH'] = prepend_bin_path(os.environ.copy(), prefix, prepend_prefix)['PATH']
Comment on lines +987 to +990
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No idea why this is prepend_prefix=True for all platforms.
For now, I left it that way and only changed it to False in build.write_build_scripts where I needed it.
I did not look at the other places where path_prepended is used. For me, this is also out of scope for this PR, but I'd like us to fix it, hence the added FIXME-note.

try:
yield
finally:
Expand Down
3 changes: 3 additions & 0 deletions conda_build/windows.py
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,9 @@ def write_build_scripts(m, env, bld_bat):


def build(m, bld_bat, stats, provision_only=False):
# TODO: Prepending the prefixes here should probably be guarded by
# if not m.activate_build_script:
# Leaving it as is, for now, since we need a quick, non-disruptive patch release.
with path_prepended(m.config.host_prefix):
with path_prepended(m.config.build_prefix):
env = environ.get_dict(m=m)
Expand Down
20 changes: 20 additions & 0 deletions news/4665-fix-activation-path
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
### Enhancements

* <news item>

### Bug fixes

* Fix build/host environment activation broken in >=3.23.0,<=3.23.2
* Add PREFIX/bin to PATH on Windows and remove PREFIX root from PATH on Unix

### Deprecations

* <news item>

### Docs

* <news item>

### Other

* <news item>
40 changes: 39 additions & 1 deletion tests/test_api_build.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
from conda_build.render import finalize_metadata
from conda_build.utils import (copy_into, on_win, check_call_env, convert_path_for_cygwin_or_msys2,
package_has_file, check_output_env, get_conda_operation_locks, rm_rf,
walk, env_var, FileNotFoundError)
prepend_bin_path, walk, env_var, FileNotFoundError)
from conda_build.os_utils.external import find_executable
from conda_build.exceptions import (DependencyNeedsBuildingError, CondaBuildException,
OverLinkingError, OverDependingError)
Expand Down Expand Up @@ -1652,3 +1652,41 @@ def assert_keyword(keyword):
assert_keyword('<hidden>')
finally:
os.environ.pop(token)


@pytest.mark.slow
def test_activated_prefixes_in_actual_path(testing_config, testing_metadata):
"""
Check if build and host env are properly added to PATH in the correct order.
Do this in an actual build and not just in a unit test to avoid regression.
Currently only tests for single non-"outputs" recipe with build/host split
and proper env activation (Metadata.is_cross and Config.activate both True).
"""
file = "env-path-dump"
testing_metadata.config.activate = True
meta = testing_metadata.meta
meta["requirements"]["host"] = []
meta["build"]["script"] = [
f"echo %PATH%>%PREFIX%/{file}" if on_win else f"echo $PATH>$PREFIX/{file}"
]
outputs = api.build(testing_metadata)
env = {"PATH": ""}
# We get the PATH entries twice: (which we should fix at some point)
# 1. from the environment activation hooks,
# 2. also beforehand from utils.path_prepended at the top of
# - build.write_build_scripts on Unix
# - windows.build on Windows
# And apparently here the previously added build env gets deactivated
# from the activation hook, hence only host is on PATH twice.
Comment on lines +1679 to +1680
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe CONDA_SHLVL is not reset for Windows builds? IDK. I didn't check since debugging for Windows is rather time-consuming on my end.
This can be investigated later; not something we have to do in this PR.

prepend_bin_path(env, testing_metadata.config.host_prefix)
if not on_win:
prepend_bin_path(env, testing_metadata.config.build_prefix)
prepend_bin_path(env, testing_metadata.config.host_prefix)
prepend_bin_path(env, testing_metadata.config.build_prefix)
expected_paths = [path for path in env["PATH"].split(os.pathsep) if path]
actual_paths = [
path
for path in package_has_file(outputs[0], file).strip().split(os.pathsep)
if path in expected_paths
]
assert actual_paths == expected_paths