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.

Sign up for GitHub

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

Jump to bottom

Allow for requirements files to differ per platform #531

Merged
merged 2 commits into from
Apr 22, 2022
Merged

Allow for requirements files to differ per platform #531

merged 2 commits into from
Apr 22, 2022

Conversation

alexeagle
Copy link
Contributor

As a common example, we need a compiled requirements file for linux that differs from mac os

PR Checklist

Please check if your PR fulfills the following requirements:

  • Does not include precompiled binaries, eg. .par files. See CONTRIBUTING.md for info
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@google-cla google-cla bot added the cla: yes label Sep 8, 2021
@alexeagle alexeagle force-pushed the platform_requirements branch 2 times, most recently from 82ebb17 to 956e1bf Compare September 8, 2021 19:20
@UebelAndre UebelAndre mentioned this pull request Oct 22, 2021
Open
@alexeagle alexeagle force-pushed the platform_requirements branch from 956e1bf to be0f661 Compare January 5, 2022 15:56
@alexeagle alexeagle force-pushed the platform_requirements branch 2 times, most recently from 232a6f3 to 8bf2bf4 Compare April 22, 2022 12:33
@alexeagle alexeagle marked this pull request as ready for review April 22, 2022 12:36
@alexeagle alexeagle requested review from hrfuller and removed request for brandjon, lberki and thundergolfer April 22, 2022 12:36
@alexeagle
Allow for requirements files to differ per platform
f480ebe 
As a common example, we need a compiled requirements file for linux that differs from mac os
@alexeagle alexeagle force-pushed the platform_requirements branch 3 times, most recently from 539cd04 to f6c0e46 Compare April 22, 2022 13:02
@alexeagle alexeagle requested a review from mattem April 22, 2022 13:10
@alexeagle
Add pip_repository documentation
0c811e0 
The macros are leaky and otherwise you have to read sources to find out about the kwargs

Fixes bazelbuild#384
@alexeagle alexeagle force-pushed the platform_requirements branch from f6c0e46 to 0c811e0 Compare April 22, 2022 13:11
@alexeagle alexeagle mentioned this pull request Apr 22, 2022
Closed
12 tasks
@mattem mattem merged commit ae7a267 into bazelbuild:main Apr 22, 2022
@alexeagle alexeagle deleted the platform_requirements branch April 22, 2022 14:10
phst added a commit to phst/rules_elisp that referenced this pull request Apr 22, 2022
@phst
Use per-platform pip requirements.
52cda39 
These are now natively supported by rules_python,
cf. bazelbuild/rules_python#531.  This means we can now
remove our hand-rolled support for per-platform requirements.
@phst phst mentioned this pull request Apr 22, 2022
Merged
@UebelAndre
Copy link
Contributor

I think this is slightly incorrect. Instead of selecting the requirements based on the host, a dependency graph should be created for each target platform, so if I build on a macos host and target a linux platform (maybe for a docker container), I get the right set of requirements. Right now it'd be wrong unless my dependency graph happens to be the same.

phst added a commit to phst/rules_elisp that referenced this pull request Apr 22, 2022
@phst
Use per-platform pip requirements.
8a2a7c4 
These are now natively supported by rules_python,
cf. bazelbuild/rules_python#531.  This means we can now
remove our hand-rolled support for per-platform requirements.
@alexeagle
Copy link
Contributor Author

@UebelAndre the actual install has to happen on the target platform so in a docker container you'd copy in the Linux requirements and use pip_install. There's no Mac outputs of pip_parse that can be useful there.

@alexeagle
Copy link
Contributor Author

Note to self: this PR only updated the pip_parse/incremental=True codepath to use the platform-specific lockfile. pip_install usage still ends up with the unlocked requirements, and isn't platform-aware.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattem mattem mattem approved these changes

@hrfuller hrfuller Awaiting requested review from hrfuller

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alexeagle @UebelAndre @mattem