fix: git URLs working with bootstrap-parallel #842
Conversation
src/fromager/commands/build.py
Outdated
| ) | ||
| for node in buildable_nodes: | ||
| req = Requirement(f"{node.canonicalized_name}=={node.version}") | ||
| # Try to get the original top-level requirement (which may include git URLs) |
There was a problem hiding this comment.
I think this is pretty close to what we want, but there's a subtly unsafe aspect to this implementation.
The same package can appear in the graph multiple times, with different versions. This implementation assumes that if a package appears as a top-level node, that top-level specifier is always the rule we should use to build it. It also assumes that that top-level rule includes the right version specifier so that we aren't re-resolving the version here. Both of those assumptions can be broken.
I think what you want to do is only use the top-level requirement specifier if the current node being processed is a child of the top-level and the rule leading to this node includes a URL. Otherwise, you want to use the version that has already been resolved.
That will fix the problem for URL-based rules, and it avoids introducing opportunities to either not build enough versions (for multiple appearances of the same package) or to build an unpredictable version (for top-level rules that mention a package but do not pin the version). By the time we get here the version we want is already resolved, so for both of those cases we should just use the version in the node.
There was a problem hiding this comment.
Thanks for pointing this out! I addressed the issue. Let me know if you need further fixes.
There was a problem hiding this comment.
@dhellmann I think this is resolved. Can you please take another look?
iangelak
force-pushed
the
761-fix-bootstrap-parallel-git-urls
branch
from
9c42f83 to
310987b
Compare
src/fromager/dependency_graph.py
Outdated
| """ | ||
| root = self.get_root_node() | ||
| for edge in root.children: | ||
| if edge.destination_node.key == node.key: |
There was a problem hiding this comment.
I think you can say edge.destination_node is node?
You could also look through node.parents for an edge with a destination node that is the root node. I don't know which is likely to be more efficient in terms of iterative over fewer items on average.
src/fromager/sources.py
Outdated
| # Parse ref from URL if it's in the form repo@ref | ||
| from urllib.parse import urlparse | ||
|
|
||
| if ref is None: |
There was a problem hiding this comment.
Where do we have a call that's cloning from a URL where the caller does not pass a valid ref?
iangelak
force-pushed
the
761-fix-bootstrap-parallel-git-urls
branch
2 times, most recently
from
177d4e4 to
08c4243
Compare
|
Let me know if you need something to be done differently. |
iangelak
force-pushed
the
761-fix-bootstrap-parallel-git-urls
branch
from
08c4243 to
9055403
Compare
iangelak
force-pushed
the
761-fix-bootstrap-parallel-git-urls
branch
from
9055403 to
06c1566
Compare
|
@iangelak The git commit message is very long and not necessary. What we want is few sentences which will explain what is this PR going fix. Please fix the commit message. Thanks. |
iangelak
force-pushed
the
761-fix-bootstrap-parallel-git-urls
branch
from
06c1566 to
ec55f54
Compare
Build-parallel reconstructed requirements as name==version, losing git URL info and causing invalid URL errors. Fixes: python-wheel-build#761 Co-authored-by: Claude <claude@anthropic.com> Signed-off-by: Ioannis Angelakopoulos <iangelak@redhat.com>
iangelak
force-pushed
the
761-fix-bootstrap-parallel-git-urls
branch
from
ec55f54 to
5687cf7
Compare
|
Ahh, my approval merged the PR. I assumed that it will wait for @dhellmann 's approval. |
Now that you're a maintainer, approving a PR causes it to merge. |
dhellmann
left a comment
dhellmann
left a comment
There was a problem hiding this comment.
@iangelak , thank you for this! I had a couple of minor comments, since @LalatenduMohanty tagged me after the merge. No big deals, but if you'd fix the doc typo before we tag a release that would be good.
|
|
||
| .. code-block:: console | ||
| $ fromager bootstrap-parallel stevedore @ git+https://github.com/openstack/stevedore.git |
There was a problem hiding this comment.
I think this syntax isn't right because there are spaces. It would at least need to be quoted to be processed properly, as was done in the test script. Please submit a follow up PR to fix that.
| $ fromager bootstrap-parallel stevedore @ git+https://github.com/openstack/stevedore.git | |
| $ fromager "bootstrap-parallel stevedore @ git+https://github.com/openstack/stevedore.git" |
| download_path = ctx.work_dir / f"{req.name}-{version}" / f"{req.name}-{version}" | ||
| download_path.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| # Parse ref from URL if it's in the form repo@ref |
There was a problem hiding this comment.
I might have put this logic in download_git_source to avoid having multiple callers have to implement it, but we can refactor that separately.
3 participants
Build-parallel reconstructed requirements as name==version, losing git URL info and causing invalid URL errors.
Fixes: #761