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

Make get_default_canonical_id public #22742

Closed
wants to merge 14 commits into from
Closed

Make get_default_canonical_id public #22742

wants to merge 14 commits into from

Conversation

Silic0nS0ldier
Copy link
Contributor

@Silic0nS0ldier Silic0nS0ldier commented Jun 14, 2024 •
edited
Loading

When repository_ctx.download or repository_ctx.download_and_extract are not given an explicit canonical_id the default behaviour can lead to some counterintuitive results (e.g. URL changed but old asset restored from cache due to unchanged checksum).

This PR seeks to bring greater attention to canonical_id in these low level API (relative to http_archive which uses get_default_canonical_id by default).

URLs are usually the most appropriate canonical_id choice, so get_default_canonical_id has been added to the public API and sample usage added to documentation.

Related to #22652

TODO

I need some pointers to make these happen.

  • Add get_default_canonical_id to the Bazel docs (I believe there are other loaded API that is missing from docs in one way or another).
  • Confirm examples render correctly in docs.

@github-actions github-actions bot added team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. awaiting-review PR is awaiting review from an assigned reviewer labels Jun 14, 2024
@sgowroji sgowroji added awaiting-user-response Awaiting a response from the author and removed awaiting-review PR is awaiting review from an assigned reviewer labels Jun 15, 2024
@Silic0nS0ldier Silic0nS0ldier force-pushed the make-get-default-canonical-id-public branch from 65d1eac to f349322 Compare June 15, 2024 15:34
@Silic0nS0ldier
Copy link
Contributor Author

Broken test should be fixed, and I've got the documentation sorted.

@Silic0nS0ldier
Copy link
Contributor Author

Maybe this time, since I apparently missed a default lockfile test. 🤞

@Silic0nS0ldier Silic0nS0ldier requested a review from fmeum July 1, 2024 08:15
@fmeum fmeum requested a review from meteorcloudy July 1, 2024 09:02
@meteorcloudy meteorcloudy added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-user-response Awaiting a response from the author labels Jul 2, 2024
@meteorcloudy
Copy link
Member

@Silic0nS0ldier Can you please add an integration test to make sure this API would work as expected?
Maybe in https://cs.opensource.google/bazel/bazel/+/master:src/test/shell/bazel/external_integration_test.sh

meteorcloudy
meteorcloudy requested changes Jul 4, 2024
View reviewed changes
Copy link
Member

@meteorcloudy meteorcloudy left a comment

Choose a reason for hiding this comment

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

Please add a simple integration test

@iancha1992
Copy link
Member

Also, looks like there is a conflict with a file. Could you please take a look? Thanks!
cc: @meteorcloudy @Silic0nS0ldier @fmeum

@Silic0nS0ldier
Copy link
Contributor Author

Conflict resolved. Working on integration test now.

@Silic0nS0ldier
Copy link
Contributor Author

Integration test added, but I noticed BAZEL_HTTP_RULES_URLS_AS_DEFAULT_CANONICAL_ID. Do we want this behaviour outside of the builtin HTTP rules?

@meteorcloudy
Copy link
Member

I guess we do want to keep it consistent? @fmeum WDYT?

@fmeum
Copy link
Collaborator

fmeum commented Jul 17, 2024

This is tough... The name suggests it only affects the http_* rules, but it's also surprising if the central knob doesn't control this for all similar rules. Maybe we interpret this as meaning "http_* like rules"?

@meteorcloudy
Copy link
Member

It's not ideal, but I think it's fine with proper documentation?

@meteorcloudy meteorcloudy added awaiting-user-response Awaiting a response from the author and removed awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally labels Jul 25, 2024
@Silic0nS0ldier
Copy link
Contributor Author

I'll make sure this is properly documented. I'm also going to move the examples over to get_default_canonical_id, having .download and .download_and_extract link to its documentation.

Side note, I noticed download_remote_files (a utility for downloading files which are overlaid into the repo) is used in the definition of http_archive but does not adhere to BAZEL_HTTP_RULES_URLS_AS_DEFAULT_CANONICAL_ID. This is in conflict with documented behaviour. Worse is that it is using the canonical ID of the 'main' file which seems like a bug (same canonical ID used for multiple files).

@fmeum
Copy link
Collaborator

fmeum commented Jul 28, 2024

It makes sense to change the behavior, although remote files are mostly used to power BCR use cases, where the URLs are not user-specified. It may even be more efficient to not use canonical IDs for them as many remote overlay files won't change between module versions, but their URLs will.

@Silic0nS0ldier
Copy link
Contributor Author

Documentation has been updated.

I won't make any to download_remote_files in this PR (outside of scope).

meteorcloudy
meteorcloudy approved these changes Aug 7, 2024
View reviewed changes
Copy link
Member

@meteorcloudy meteorcloudy left a comment

Choose a reason for hiding this comment

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

Thanks!

@meteorcloudy meteorcloudy added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-user-response Awaiting a response from the author labels Aug 7, 2024
@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Aug 14, 2024
fmeum pushed a commit to fmeum/bazel that referenced this pull request Aug 22, 2024
@Silic0nS0ldier @fmeum
Make get_default_canonical_id public
64dcfff 
When `repository_ctx.download` or `repository_ctx.download_and_extract` are not given an explicit `canonical_id` the default behaviour can lead to some counterintuitive results (e.g. URL changed but old asset restored from cache due to unchanged checksum).

This PR seeks to bring greater attention to `canonical_id` in these low level API (relative to `http_archive` which uses `get_default_canonical_id` by default).

URLs are usually the most appropriate `canonical_id` choice, so `get_default_canonical_id` has been added to the public API and sample usage added to documentation.

Related to bazelbuild#22652

I need some pointers to make these happen.

- [x] Add `get_default_canonical_id` to the Bazel docs (I believe there are other `load`ed API that is missing from docs in one way or another).
- [x] Confirm examples render correctly in docs.

Closes bazelbuild#22742.

PiperOrigin-RevId: 662822225
Change-Id: I1617aa16a62da2d8dff2034fef8ca19aecd33d58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmeum fmeum fmeum approved these changes

@meteorcloudy meteorcloudy meteorcloudy approved these changes

Assignees
No one assigned
Labels
team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Silic0nS0ldier @meteorcloudy @iancha1992 @fmeum @sgowroji