Fix: Build environments not activated #4665

mbargull · 2022-12-03T17:58:16Z

Description

In conda-forge/conda-feedstock#181 (comment) we encountered unexpected failures for PyPy variants of the builds with

D:/bld/conda_1669913819035/_h_env/python.exe: error while loading shared libraries: libpypy3.9-c.dll: cannot open shared object file: No such file or directory

It turned out, this happened because the build environments were not activated at all.
The correct Python interpreter was launched because the build script used $PYTHON.
The CPython variants built fine "by chance" only since our CPython builds carry library lookup patches to also function without an activated environment and other programs needed for the build were already on PATH from outside the build environment.

This happened due to a regression caused by cecb031 .
Previously, conda's memoized bailed out from caching anything because of the additionally passed non-hashable dict.
Memoizing with a MetaData instance as a key is problematic since that object/its members are highly mutable during conda-build's work.

The actual fix in commit 7f047c7 just removes the memoization.
I also added two guards to avoid regressions:

Make MetaData non-hashable in 9fd7165.
Add a test in 9fde4c1 + 060f10f to check if the prefixes are added to PATH as expected.

Note that the added test did not replicate the behavior from the conda-forge build entirely.
During the test runs, the prefixes were only added in a wrong order (possibly due to merged build/host at the time of memoization) but they were not completely omitted like in the conda-feedstock build.
These subtleties can be expected due to different setups and different times of memoization.
It is also likely that the early memoization changed other parts of the builds; changes to PATH were just most visible.

This PR also adds PREFIX/bin to PATH on Windows and removes PREFIX root from PATH on Unix.
Those were two bugs I noticed broke the added test case.
Fixing that should be non-controversial without any downsides, really (those are plain bugs anyway).

Checklist - did you ...

Add a file to the news directory (using the template) for the next release's release notes?
Add / update necessary tests?
Add / update outdated documentation?

Its members are very dynamic so it is problematic to use it for cached calculations.

Needed to set .activate=True on the MetaData's own .config

mbargull · 2022-12-03T23:41:06Z

I updated the OP with explanations for what happened and how this is mitigated.

cc @jezdez, @kenodegard
Another quick patch release would be highly appreciated since this is actually a severe issue with many unknowns in what parts under which build configurations it impacts.

- Prefix "bin" directory get added on all platforms - Prefix root directory should only be added on Windows

mbargull · 2022-12-04T11:27:01Z

conda_build/build.py

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


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

mbargull · 2022-12-04T11:28:43Z

conda_build/utils.py

@@ -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']


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.

mbargull · 2022-12-04T11:32:37Z

conda_build/utils.py

+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']


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.

mbargull · 2022-12-04T11:35:52Z

tests/test_api_build.py

+    #        And apparently here the previously added build env gets deactivated
+    #        from the activation hook, hence only host is on PATH twice.


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.

mbargull · 2022-12-04T11:42:17Z

This PR also adds PREFIX/bin to PATH on Windows and removes PREFIX root from PATH on Unix.
Those were two bugs I noticed broke the added test case.
Fixing that should be non-controversial without any downsides, really (those are plain bugs anyway).

travishathaway

Hi @mbargull

Thanks for this pull request. So far everything looks okay with this pull request, and I will be happy to approve, but I would really appreciate it if you could add a descriptive doc string to your test explaining exactly why it exists and perhaps its shortcomings as you mentioned.

We are trying hard to make sure the tests in conda-build are more maintainable and these simple descriptions are very helpful for future maintainers.

Thank you! ✌️

mbargull · 2022-12-05T13:19:43Z

@travishathaway, thanks for the review! Addressed in 1ebe72c .

We are trying hard to make sure the tests in conda-build are more maintainable and these simple descriptions are very helpful for future maintainers.

Glad to hear that process started! 😸

dholth

LGTM

dbast · 2022-12-05T13:36:06Z

@travishathaway I don't mind doc strings in test cases... more important for me: test cases follow a given/when/then or setup/run-subject-under-test/compare-outcome structure (which is given here), which often makes the code self explanatory beyond to what can described in english via doctsrings.

mbargull · 2022-12-05T14:58:24Z

Failure in Tests / linux (3.9, canary, parallel) is unrelated...

... but it's still odd, since after
1053650#diff-e52e4ddd58b7ef887ab03c04116e676f6280b824ab7469d5d3080e5cba4f2128R126-R144
we shouldn't see /usr/share/miniconda/envs/test/conda-bld/work used in these kinds of tests anymore...
Something about the fixture/its "autouse" doesn't seem to work properly sigh...

=================================== FAILURES ===================================
__________________ test_no_satisfiable_variants_raises_error ___________________
[gw0] linux -- Python 3.9.15 /usr/share/miniconda/envs/test/bin/python
Traceback (most recent call last):
  File "/usr/share/miniconda/envs/test/lib/python3.9/shutil.py", line 825, in move
    os.rename(src, real_dst)
FileNotFoundError: [Errno 2] No such file or directory: '/usr/share/miniconda/envs/test/conda-bld/work' -> '/usr/share/miniconda/envs/test/conda-bld/abc_1670250124813/work'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/runner/work/conda-build/conda-build/tests/test_variants.py", line 106, in test_no_satisfiable_variants_raises_error
    api.render(recipe, permit_unsatisfiable_variants=True)
  File "/home/runner/work/conda-build/conda-build/conda_build/api.py", line 38, in render
    metadata_tuples = render_recipe(recipe_path, bypass_env_check=bypass_env_check,
  File "/home/runner/work/conda-build/conda-build/conda_build/render.py", line 827, in render_recipe
    m.config.compute_build_id(m.name(), m.version(), reset=reset_build_id)
  File "/home/runner/work/conda-build/conda-build/conda_build/config.py", line 615, in compute_build_id
    shutil.move(old_dir, work_dir)
  File "/usr/share/miniconda/envs/test/lib/python3.9/shutil.py", line 845, in move
    copy_function(src, real_dst)
  File "/usr/share/miniconda/envs/test/lib/python3.9/shutil.py", line 444, in copy2
    copyfile(src, dst, follow_symlinks=follow_symlinks)
  File "/usr/share/miniconda/envs/test/lib/python3.9/shutil.py", line 264, in copyfile
    with open(src, 'rb') as fsrc:
FileNotFoundError: [Errno 2] No such file or directory: '/usr/share/miniconda/envs/test/conda-bld/work'

jakirkham · 2022-12-05T19:43:20Z

It sounds like we would like to capture this in a patch release of conda-build (like 3.23.3). Is that something that could be started?

jezdez · 2022-12-06T10:56:55Z

Yes

test: Add test to inspect actual PATH during build

9fde4c1

conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Dec 3, 2022

mbargull added 5 commits December 3, 2022 20:31

Fix: MetaData should never be used for memoization

9fd7165

Its members are very dynamic so it is problematic to use it for cached calculations.

Fix: Remove invalid memoization for MetaData

7f047c7

test: Fix test_activated_prefixes_in_actual_path

060f10f

Needed to set .activate=True on the MetaData's own .config

lint

ba9c666

Add news/4665-fix-activation-path

73c2802

mbargull marked this pull request as ready for review December 3, 2022 23:36

mbargull mentioned this pull request Dec 4, 2022

MNT: rerender conda-forge/pypy3.6-feedstock#97

Closed

1 task

mbargull marked this pull request as draft December 4, 2022 09:28

Fix non-activate prepend_bin_path, path_prepended

998597c

- Prefix "bin" directory get added on all platforms - Prefix root directory should only be added on Windows

mbargull force-pushed the fix-activation-path branch from 36133a6 to 998597c Compare December 4, 2022 11:14

mbargull marked this pull request as ready for review December 4, 2022 11:14

mbargull commented Dec 4, 2022

View reviewed changes

mbargull mentioned this pull request Dec 5, 2022

Fix: Remove invalid memoization for MetaData conda-forge/conda-build-feedstock#193

Merged

5 tasks

jezdez requested review from kenodegard, dbast and dholth December 5, 2022 10:36

travishathaway requested changes Dec 5, 2022

View reviewed changes

test: Add description in test docstring

1ebe72c

dholth approved these changes Dec 5, 2022

View reviewed changes

dbast approved these changes Dec 5, 2022

View reviewed changes

dbast closed this Dec 5, 2022

dbast reopened this Dec 5, 2022

travishathaway approved these changes Dec 5, 2022

View reviewed changes

mbargull mentioned this pull request Dec 5, 2022

Reduce flakiness of CI test runs #4653

Merged

3 tasks

dbast merged commit 7e9d123 into conda:main Dec 5, 2022

mbargull mentioned this pull request Dec 7, 2022

Re-enable Windows and PyPy builds conda-forge/conda-feedstock#187

Closed

5 tasks

github-actions bot added the locked [bot] locked due to inactivity label Dec 7, 2023

github-actions bot locked as resolved and limited conversation to collaborators Dec 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Build environments not activated #4665

Fix: Build environments not activated #4665

mbargull commented Dec 3, 2022 •

edited

Loading

mbargull commented Dec 3, 2022

mbargull Dec 4, 2022

mbargull Dec 4, 2022

mbargull Dec 4, 2022

mbargull Dec 4, 2022

mbargull commented Dec 4, 2022

travishathaway left a comment •

edited

Loading

mbargull commented Dec 5, 2022

dholth left a comment

dbast commented Dec 5, 2022

mbargull commented Dec 5, 2022

jakirkham commented Dec 5, 2022

jezdez commented Dec 6, 2022

		# And apparently here the previously added build env gets deactivated
		# from the activation hook, hence only host is on PATH twice.

Fix: Build environments not activated #4665

Fix: Build environments not activated #4665

Conversation

mbargull commented Dec 3, 2022 • edited Loading

Description

Checklist - did you ...

mbargull commented Dec 3, 2022

mbargull Dec 4, 2022

Choose a reason for hiding this comment

mbargull Dec 4, 2022

Choose a reason for hiding this comment

mbargull Dec 4, 2022

Choose a reason for hiding this comment

mbargull Dec 4, 2022

Choose a reason for hiding this comment

mbargull commented Dec 4, 2022

travishathaway left a comment • edited Loading

Choose a reason for hiding this comment

mbargull commented Dec 5, 2022

dholth left a comment

Choose a reason for hiding this comment

dbast commented Dec 5, 2022

mbargull commented Dec 5, 2022

jakirkham commented Dec 5, 2022

jezdez commented Dec 6, 2022

mbargull commented Dec 3, 2022 •

edited

Loading

travishathaway left a comment •

edited

Loading