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

Set bazel_compatibility for rules_sh #1225

Merged
merged 4 commits into from
Dec 26, 2023
Merged

Set bazel_compatibility for rules_sh #1225

merged 4 commits into from
Dec 26, 2023

Conversation

avdv
Copy link
Contributor

@avdv avdv commented Dec 21, 2023

No description provided.

@fmeum fmeum added the USE-WITH-CAUTION-skip-modification-check Allow modifying checked-in modules, reproducible dependency resolution should not be compromised label Dec 21, 2023
@fmeum
Copy link
Contributor

fmeum commented Dec 21, 2023

@Wyverald I added the label to allow modifications. I think that this use case is fine, but would value another pair of eyes on this.

@fmeum fmeum added the skip-url-stability-check Skip the URL stability check for the PR label Dec 21, 2023
@Wyverald
Copy link
Member

looks like you forgot the actual patch file?

I also think this is probably warranted, but just to confirm, doesn't your new lockfile turn this off forever? (or at least until the download cache somehow gets cleared)

@fmeum
Copy link
Contributor

fmeum commented Dec 21, 2023

Yes, these kinds of changes are not going to be effective with the new lockfile format. I could see a point for moving Bazel compatibility into metadata.json instead. If we want to do automated compatibility testing at some point, this would probably be needed anyway as we wouldn't want to edit the MODULE.bazel files.

@Wyverald
Copy link
Member

I could see a point for moving Bazel compatibility into metadata.json instead.

yeah, I agree. cc @meteorcloudy

@avdv avdv force-pushed the rules_sh-0.3.0-compat branch 2 times, most recently from 585a21b to ae126ea Compare December 22, 2023 09:26
@avdv
Copy link
Contributor Author

avdv commented Dec 22, 2023

I could see a point for moving Bazel compatibility into metadata.json instead.

Yes, intuitively this was where I first thought this has to be added.

I am not sure this is the right thing to do, but there is an incompatibility on amd64 darwin for this module which makes it fail on Bazel 7, which is blocking PR #1223 currently.

Is the bazel_compatibility setting a good use case for this?

@avdv avdv marked this pull request as ready for review December 22, 2023 16:21
@Wyverald Wyverald merged commit a3596b8 into bazelbuild:main Dec 26, 2023
11 checks passed
@avdv avdv deleted the rules_sh-0.3.0-compat branch January 8, 2024 12:26
@avdv avdv mentioned this pull request Jan 8, 2024
Merged
antonovvk pushed a commit to antonovvk/bazel-central-registry that referenced this pull request Jan 21, 2024
@avdv @Wyverald @antonovvk
Set bazel_compatibility for rules_sh (bazelbuild#1225)
8788fac 
* Set bazel_compatibility for rules_sh

* fix version constraint

* Use Bazel 6.x

---------

Co-authored-by: Xùdōng Yáng <wyverald@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Wyverald Wyverald Wyverald approved these changes

Assignees
No one assigned
Labels
skip-url-stability-check Skip the URL stability check for the PR USE-WITH-CAUTION-skip-modification-check Allow modifying checked-in modules, reproducible dependency resolution should not be compromised
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@avdv @fmeum @Wyverald