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

Handle no-conda environment, including msgpack #2409

Merged
merged 4 commits into from
Jun 12, 2023
Merged

Handle no-conda environment, including msgpack #2409

merged 4 commits into from
Jun 12, 2023

Conversation

phyy-nx
Copy link
Member

@phyy-nx phyy-nx commented May 2, 2023

Relevant e.g. for virtualenv builds, using a facility provided python. Uses the msgpack from the bootstrap 'hot' step.

Note in #2319, specifically fb95810, the msgpack3-1-1 reference was removed. The confusion here was that the comment said it was only needed for Python2.7. Turns out that isn't strictly true, as the virtualenv build path uses this hot package.

Someone should make a CI that exercises the virtualenv build path :)

@phyy-nx phyy-nx requested a review from ndevenish May 2, 2023 21:05
@codecov
Copy link

codecov bot commented May 2, 2023

Codecov Report

Merging #2409 (0a87800) into main (f117929) will decrease coverage by 0.07%.
The diff coverage is n/a.

❗ Current head 0a87800 differs from pull request most recent head df5c159. Consider uploading reports for the commit df5c159 to get more accurate results

@@            Coverage Diff             @@
##             main    #2409      +/-   ##
==========================================
- Coverage   78.62%   78.56%   -0.07%     
==========================================
  Files         603      603              
  Lines       73644    73644              
  Branches    10005    10005              
==========================================
- Hits        57903    57855      -48     
- Misses      13612    13658      +46     
- Partials     2129     2131       +2     

SConscript Show resolved Hide resolved
@ndevenish
Copy link
Member

Wouldn't this be better handled by e.g. injecting the include path into the build environment at the same point that is responsible for installing the """"""hot"""""" dependency?

@phyy-nx
Copy link
Member Author

phyy-nx commented May 3, 2023

I think you are saying add the include directory in bootstrap if use_conda isn't set? That could work too. This PR kinda just puts us back where we were, so it was lower effort :)

FWIW, the old 'hot' mechanism in bootstrap was exactly for this purpose: distributing packages that are frozen due to some functionality being needed for a specific purpose. Perhaps it would be better to call it a 'cold' mechanism :P

If it helps, here's a list of pseudocode steps this build procedure uses (this one is for SACLA):

  • bootstrap hot update
  • module load python 3.8, openmpi, hdf5. These are custom libraries from the facility.
  • python3 -m venv --system-site-packages env. This sets up the virtual environment
  • source env/bin/activate
  • pip install just all the things
  • bootstrap build

@ndevenish ndevenish enabled auto-merge (squash) June 12, 2023 09:31
@ndevenish ndevenish disabled auto-merge June 12, 2023 09:47
@ndevenish ndevenish merged commit 9941fef into main Jun 12, 2023
4 of 6 checks passed
@ndevenish ndevenish deleted the nocondafix branch June 12, 2023 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants