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

chore: use coreutils hashsum #333

Merged
merged 16 commits into from
Mar 10, 2023
Merged

chore: use coreutils hashsum #333

merged 16 commits into from
Mar 10, 2023

Conversation

thesayyn
Copy link
Collaborator

No description provided.

@gregmagolan
Copy link
Contributor

gregmagolan commented Jan 16, 2023

Should document the new dep on the core_utils toolchain for the hashes rule.

EDIT: actually the only way to make it non-breaking is to always register the toolchain like we did for the new copy_to_directory toolchain: https://github.com/aspect-build/bazel-lib/blob/4dc36a97f23c3a925142d4bd923be029ee1fe522/lib/repositories.bzl#L38

@gregmagolan
Copy link
Contributor

gregmagolan commented Jan 17, 2023

Will also need to add a test for hashes to e2e/bzlmod & e2e/workspace for this one :)

@thesayyn
Copy link
Collaborator Author

done.

@thesayyn
Copy link
Collaborator Author

adding derek as a reviewer as he knows the repo and bzlmod

@kormide
Copy link
Collaborator

kormide commented Jan 17, 2023

I think you could add to the existing e2e/workspace and e2e/bzlmod, just one usage (e.g., a genrule) as a smoke test that workspace/e2e users are able to use the toolchain. We do the same in those e2es for other toolchains. For the most part unit tests can exercise coreutils, and eventually we'll be running all unit tests with bzlmod enabled on ci, so you don't need to worry about exercising it fully in an e2e.

@gregmagolan
Copy link
Contributor

Yup. Agreed that e2e/bzlmod & e2e/workspace are just smoke tests. Since we're not in a rush to land this @thesayyn we may as well take the opportunity to combine e2e/workspace & e2e/bzlmod into a single e2e/workspace and add the bzlmodEnabled: [true, false] pattern to CI for bazel-lib which @kormide just rolled out in rules_js: https://github.com/aspect-build/rules_js/blob/047decb02a7e1497d23cfb9b239b56778ea19f4d/.github/workflows/ci.yaml#L132

@gregmagolan
Copy link
Contributor

@alexeagle is going to configure our preferred pattern for bzlmod/workspace smoketest e2e in rules_swc. Lets wait until that lands and then copy it over here.

@alexeagle
Copy link
Collaborator

Working on that right now...

@alexeagle
Copy link
Collaborator

@alexeagle
Copy link
Collaborator

@thesayyn should be unblocked now

@gregmagolan gregmagolan added the tech debt Implied cost of additional rework caused by choosing an quick/easy solution earlier label Feb 6, 2023
@thesayyn
Copy link
Collaborator Author

oh, I missed this one. I'll takeover thanks!

@thesayyn
Copy link
Collaborator Author

now, this is more of a coreutils + e2e cleanup.

@thesayyn
Copy link
Collaborator Author

this is ready now

@kormide
Copy link
Collaborator

kormide commented Feb 21, 2023

  • Can we rename e2e/workspace to e2e/smoke? (.bcr/presubmit.yml also needs to reference this target)`.
  • Each e2e should also have a WORKSPACE.bzlmod file, which tells Bazel to ignore WORKSPACE when bzlmod is enabled, and any repository rules can't be set up under MODULE.bazel go in there.

e2e/workspace/MODULE.bazel Outdated Show resolved Hide resolved
@thesayyn thesayyn merged commit e318673 into main Mar 10, 2023
@thesayyn thesayyn deleted the hashsum branch March 10, 2023 17:37
@alexeagle
Copy link
Collaborator

This broke the windows build on mainppp

@gregmagolan
Copy link
Contributor

Looks like a small typo. #395 to fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in review tech debt Implied cost of additional rework caused by choosing an quick/easy solution earlier
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants