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

pip: Recommend pybuild-deps package for generating requirements-build.txt #780

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

slimreaper35
Copy link
Member

Maintainers will complete the following section

  • Commit messages are descriptive enough
  • Code coverage from testing does not decrease and new code is covered
  • Docs updated (if applicable)
  • Docs links in the code are still valid (if docs were updated)

Note: if the contribution is external (not from an organization member), the CI
pipeline will not run automatically. After verifying that the CI is safe to run:

Copy link
Member

@eskultety eskultety left a comment

Choose a reason for hiding this comment

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

The PR lacks context for this change, e.g. so we're moving some script from other repo to our repo, why? What's the motivation behind the change, what is it improving/addressing? Otherwise LGTM.

docs/pip.md Outdated Show resolved Hide resolved
docs/pip.md Outdated Show resolved Hide resolved
hack/pip_find_build_deps.py Outdated Show resolved Hide resolved
@chmeliik
Copy link
Contributor

chmeliik commented Jan 7, 2025

Is it time to start recommending https://github.com/bruno-fs/pybuild-deps instead?

@brunoapimentel
Copy link
Contributor

Is it time to start recommending https://github.com/bruno-fs/pybuild-deps instead?

Yes, I pushed for this at the time you filed those issues. We can revisit this now.

@slimreaper35
Copy link
Member Author

The PR lacks context for this change, e.g. so we're moving some script from other repo to our repo, why? What's the motivation behind the change, what is it improving/addressing? Otherwise LGTM.

I thought, that when we break the relationship with Cachito completely and update all the references, we should move the script for generating build dependencies here. I don't know in what shape is https://github.com/bruno-fs/pybuild-deps but we can for example have this as an alternative option mentioned in the docs.

hack/pip_find_build_deps.py Outdated Show resolved Hide resolved
@brunoapimentel
Copy link
Contributor

brunoapimentel commented Jan 7, 2025

I thought, that when we break the relationship with Cachito completely and update all the references, we should move the script for generating build dependencies here. I don't know in what shape is https://github.com/bruno-fs/pybuild-deps but we can for example have this as an alternative option mentioned in the docs.

I would rather just stop referecing the script at all and simply point to pybuild-deps, as it is a more robust and efficient solution.

(btw, the developer has already asked us to move pybuild-deps under our organization, so we in case we decide to go that way, we would become the maintainers of this project, making it more "official")

@slimreaper35
Copy link
Member Author

I would rather just stop referecing the script at all and simply point to pybuild-deps, as it is a more robust and efficient solution.
(btw, the developer has already asked us to move pybuild-deps under our organization, so we in case we decide to go that way, we would become the maintainers of this project, making it more "official")

Do you want me to do it in this PR? Or wait until we move our repo under a new organization and decide on pybuild-deps location?

@brunoapimentel
Copy link
Contributor

brunoapimentel commented Jan 7, 2025

Do you want me to do it in this PR? Or wait until we move our repo under a new organization and decide on pybuild-deps location?

I slightly preferer doing it now rather than moving the script to this repo and replacing it with pybuild-deps later. Wdyt @eskultety @a-ovchinnikov? Otherwise, let's follow up this PR as is, I'd be fine with it too.

@slimreaper35
Copy link
Member Author

slimreaper35 commented Jan 7, 2025

So let's do it here. It is a simple change in the docs. Does not make sense to "temporarily" merge those two commits.

@eskultety
Copy link
Member

Do you want me to do it in this PR? Or wait until we move our repo under a new organization and decide on pybuild-deps location?

I slightly preferer doing it now rather than moving the script to this repo and replacing it with pybuild-deps later. Wdyt @eskultety @a-ovchinnikov? Otherwise, let's follow up this PR as is, I'd be fine with it too.

Can you be more specific on what "prefer doing it now" encompasses? Do you mean ONLY referencing that external project/script in our docs OR move their whole repo under this org and become maintainers straight away? Because I am totally on board with the former while I may need a little while to think about the latter as I've never seen the project in question.

@brunoapimentel
Copy link
Contributor

Can you be more specific on what "prefer doing it now" encompasses? Do you mean ONLY referencing that external project/script in our docs OR move their whole repo under this org and become maintainers straight away? Because I am totally on board with the former while I may need a little while to think about the latter as I've never seen the project in question.

Only referencing the external project (pybuild-deps) and stop referencing the old script altogether. The reasoning is that pybuild-deps is a more robust solution, with tests and more readable code, it has a much faster resolution time and it solves problems such as conflicting versions for build dependencies that the current script does not.

The latter (adopting the project) is something we'll need to carefully consider, but it is out of scope for this PR (I only mentioned it because I wanted to show that this is not just a random external project)

@eskultety
Copy link
Member

Can you be more specific on what "prefer doing it now" encompasses? Do you mean ONLY referencing that external project/script in our docs OR move their whole repo under this org and become maintainers straight away? Because I am totally on board with the former while I may need a little while to think about the latter as I've never seen the project in question.

Only referencing the external project (pybuild-deps) and stop referencing the old script altogether. The reasoning is that pybuild-deps is a more robust solution, with tests and more readable code, it has a much faster resolution time and it solves problems such as conflicting versions for build dependencies that the current script does not.

The latter (adopting the project) is something we'll need to carefully consider, but it is out of scope for this PR (I only mentioned it because I wanted to show that this is not just a random external project)

Thanks for clarifying - ACK to that in that case.

@slimreaper35 slimreaper35 changed the title pip build dependencies pip: Recommend pybuild-deps package for generating requirements-build.txt Jan 8, 2025
Our current solution for the users who want to build Python project from
source (without wheel distributions) is to generate
`requirements-build.txt` using our custom script that lives in Cachito
project [1].

Fortunately, there is an external package (repo) that provides more
robust solution that is tested and has a faster resolution time:
-> https://github.com/bruno-fs/pybuild-deps

Update the pip documentation to start referencing the package as a
preferred and recommend way to generate `requirements-build.txt`.

---
[1]: https://github.com/containerbuildsystem/cachito/blob/master/bin/pip_find_builddeps.py

Signed-off-by: Michal Šoltis <[email protected]>
@slimreaper35 slimreaper35 added this pull request to the merge queue Jan 9, 2025
Merged via the queue into hermetoproject:main with commit 7bccc8b Jan 9, 2025
15 checks passed
@slimreaper35 slimreaper35 deleted the pip branch January 9, 2025 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants