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

Apply PIP_CONSTRAINT correctly #49

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hoodmane
Copy link
Member

@hoodmane hoodmane commented Nov 6, 2024

No description provided.

@agriyakhetarpal
Copy link
Member

agriyakhetarpal commented Nov 6, 2024

We could also do a 0.29.1 release with this change so that we don't need to use your fork in pyodide/pyodide#5162, please let me know if you'd like that!

@hoodmane
Copy link
Member Author

hoodmane commented Nov 6, 2024

We don't need to release just to merge then I can use the pyodide/pyodide-build commit on main.

@agriyakhetarpal
Copy link
Member

Sure, just re-triggered the CI failure because I'm unsure if it was flaky or not. Please feel free to merge if it passes

@agriyakhetarpal
Copy link
Member

Looks like a genuine failure.

@hoodmane
Copy link
Member Author

hoodmane commented Nov 6, 2024

Looks like a real failure to me.

@agriyakhetarpal
Copy link
Member

I'm unable to reproduce this locally on macOS, the test passes for me. It's unrelated to pytest.

@ryanking13
Copy link
Member

ryanking13 commented Nov 7, 2024

But pyodide/pyodide#5132 worked without this change. Why is this change needed?

@hoodmane
Copy link
Member Author

hoodmane commented Nov 7, 2024

I'm not entirely sure why the other fix worked, but notice that we do not pass build_env to install_reqs in any way. So if we don't set this as an enviornment variable, how would the pip install command know that we have the constraint?

@ryanking13
Copy link
Member

ryanking13 commented Nov 7, 2024

PIP_CONSTRAINT is a host environment variable, so it should already be stored in os.environ without having to pass it in from build_env.

@hoodmane
Copy link
Member Author

hoodmane commented Nov 7, 2024

When I ran it locally I found os.environ["PIP_CONSTRAINT"] raised a KeyError at this spot.

@ryanking13
Copy link
Member

Hmm, that's weird... 🤔

@hoodmane
Copy link
Member Author

hoodmane commented Nov 7, 2024

Can you try it out and see if you log os.environ["PIP_CONSTRAINT"] here? If it prints something for you maybe we can just close this.

@ryanking13
Copy link
Member

Oh, you are right. It is slightly more complicated than I thought though. So what happens is:

  1. Building packages in-tree with invoking make:
  • Makefile will read and execute Makefile.envs, so PIP_CONSTRAINT is set in host env.
  1. Building packages in-tree with invoking pyodide build-recipes.
  • Makefile.envs will not be executed, so PIP_CONSTRAINT is not set in host env, but only stored in build env
  1. Building packages out-of-tree
  • Makefile.envs will not be executed, so PIP_CONSTARINT is not set in host env, but only stored in build env

So the current change looks okay. The reason why pyodide/pyodide#5132 worked is probably because we try again with build env replace when installing requirement fails.

@@ -153,6 +153,7 @@ def _build_in_isolated_env(

# first install the build dependencies
symlink_unisolated_packages(env)
os.environ["PIP_CONSTRAINT"] = build_env["PIP_CONSTRAINT"]
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a comment that this is only for in-tree build for future readers.

For OOT build, a user needs to set PIP_CONSTRAINT if needed.

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.

3 participants