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

Convert to bzlmod, first steps. #1893

Merged
merged 1 commit into from
Feb 5, 2025
Merged

Convert to bzlmod, first steps. #1893

merged 1 commit into from
Feb 5, 2025

Conversation

hzeller
Copy link
Member

@hzeller hzeller commented Jan 27, 2025 •
edited
Loading

First steps towards exclusively using MODULE.bazel, for now a subset of changes already improving the state a lot.

  • Add MODULE.bazel and add dependencies already provided by the Bazel Central Registry.
  • Remove the above from WORKSPACE and dependency_support/{load,initialize}_external.bzl.
  • Add some patches, as some external dependencies used @rules_cc// to get cc_proto_library and cc_library, which is not available anymore in latest rules_cc (but provided by bazel itself)
  • Fallout: the compilation db currently can't be built anymore due to the compdb not compatible anymore with bzlmod. I have a plan to replace that, but not in this first CL; for now, it is just disabled.

Things for follow-up PRs:

  • move the remaining load_exernal.bzl also to MODULE.bazel, but use the repo rules to get these.
  • Upstream more tools we use to Bazel Central Registry; e.g. low hanging fruit would be to get a later version of protoc-gen-validate in there.
  • fix compilation db.

Issues: #931

@hzeller hzeller force-pushed the feature-20250127-step1-bzlmod branch from 076954e to 6564c47 Compare January 27, 2025 23:45
@hzeller hzeller marked this pull request as ready for review January 27, 2025 23:45
@hzeller hzeller requested review from grebe, proppy and mikex-oss January 27, 2025 23:45
@hzeller
Copy link
Member Author

hzeller commented Jan 27, 2025

New year, new attempt.
Adding a few bazel-knowledgeable folks as reviewers.

Also: @cdleary does this compile for you ?

@hzeller
Copy link
Member Author

hzeller commented Jan 27, 2025

Earlier work was in #1570

@hzeller
Copy link
Member Author

hzeller commented Jan 28, 2025

@proppy you checked last time how well it compiles on MacOS

copybara-service bot pushed a commit that referenced this pull request Jan 28, 2025
@hzeller @copybara-github
Fix some missing includes.
811db8c 
While in the process to figure out compilation DB choices
after #1893 , I am getting
different set of language server issues with possible alternatives.

This change helps reduce the warning noise and highlighting the
differences by fixing known include issues.

#xls-build-gardener

PiperOrigin-RevId: 720724552
@hzeller hzeller force-pushed the feature-20250127-step1-bzlmod branch 5 times, most recently from 96b84cd to 310e730 Compare January 30, 2025 00:40
@hzeller hzeller requested a review from cdleary January 30, 2025 03:04
@hzeller
Copy link
Member Author

hzeller commented Jan 30, 2025

@cdleary looks good ?

@proppy
Copy link
Member

proppy commented Jan 30, 2025

@proppy you checked last time how well it compiles on MacOS

Happy to give it a try!
(We could also re-evaluate enabling the osx CI on every PR)

@hzeller hzeller force-pushed the feature-20250127-step1-bzlmod branch 3 times, most recently from cee30d2 to f84e5ef Compare January 30, 2025 18:08
@hzeller
Copy link
Member Author

hzeller commented Jan 30, 2025

Happy to give it a try! (We could also re-evaluate enabling the osx CI on every PR)

Good idea! Enabled it temporarily in this PR, and it seems to be happy: https://github.com/google/xls/actions/runs/13058469006/job/36435356862?pr=1893

@hzeller hzeller force-pushed the feature-20250127-step1-bzlmod branch 2 times, most recently from 47258ba to 597422b Compare January 31, 2025 17:30
mikex-oss
mikex-oss approved these changes Feb 3, 2025
View reviewed changes
dependency_support/com_google_ortools/ortools-no-rulescc.patch Show resolved Hide resolved
WORKSPACE Outdated Show resolved Hide resolved
fuzztest.bazelrc Outdated Show resolved Hide resolved
@hzeller hzeller force-pushed the feature-20250127-step1-bzlmod branch from 597422b to 65b355b Compare February 3, 2025 18:53
cdleary
cdleary approved these changes Feb 4, 2025
View reviewed changes
Copy link
Collaborator

@cdleary cdleary left a comment

Choose a reason for hiding this comment

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

Thanks, confirmed this seems to be working in release environments!

@proppy
Copy link
Member

proppy commented Feb 4, 2025

Thanks, confirmed this seems to be working in release environments!

Did you also confirm on OSX? (just curious if I should still give it a try)

grebe
grebe requested changes Feb 4, 2025
View reviewed changes
.gitignore
@@ -8,3 +8,4 @@ user.bazelrc
.vscode
.cache
docs/
MODULE.bazel.lock
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI best practices seem to be to include the lockfile in VCS

Copy link
Member Author

Choose a reason for hiding this comment

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

The lock file is a large file with a lot of hashes which is hard to verify. I suspect it makes sense if one uses a different repository than the bazel-central-repository, but it does not really serve a purpose otherwise: the BCR rules are so that a version number uniquely identifies a hash as they don't have a policy to switcaroo these later.

So, I am not convinced that it brings anything to the table, though if there is compelling reason we can add it later.

Copy link
Member Author

Choose a reason for hiding this comment

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

(as discussed offline: I am happy for some follow-up PR)

MODULE.bazel Outdated Show resolved Hide resolved
dependency_support/protoc-gen-validate/no-rulescc.patch Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do newer releases of the deps you need a no-rulescc patch for do the edit for you?

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't know, probably not. The protoc-gen-validate is super-ancient in BCR (1.0.4), so I intend to send a PR for that to update to the current 1.2.1, then we can see.

xls/dev_tools/make-compilation-db.sh
s/ -stdlib=libc++/ / # otherwise, clang-tidy does not find c++ headers
EOF

echo "Sorry, currently no compilation DB with bazel 7 and bzlmod. Stay tuned."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe include a pointer to a tracking issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have a tracking issue, but I have a CL that fixes this (and also provides build_cleaner functionality) ready to go as soon as this is in.

@hzeller
Convert to bzlmod, first steps.
33cdfa1 
First steps towards exclusively using MODULE.bazel, for now
a subset of changes already improving the state a lot.

  * Add MODULE.bazel and add dependencies already provided by
    the Bazel Central Registry.
  * Remove the above from WORKSPACE and
    dependency_support/{load,initialize}_external.bzl.
  * Add some patches, as some external dependencies used
    @rules_cc// to get cc_proto_library and cc_library, which is
    not available anymore in latest rules_cc (but provided by
    bazel itself)
  * Fallout: the compilation db currently can't be built anymore
    due to the compdb not compatible anymore with bzlmod.
    I have a plan to replace that, but not in this first CL; for
    now, it is just disabled.

Things for follow-up PRs
  * move the remaining load_exernal.bzl also to MODULE.bazel, but
    use the repo rules to fetch projects.
  * Upstream more tools we use to Bazel Central Registry; e.g.
    low hanging fruit would be to get a later version of protoc-gen-validate
    in there or add lineoise.
  * Rules python and its toolchain can probably be simplified.
  * fix compilation db.

Issues: google#931
@hzeller hzeller force-pushed the feature-20250127-step1-bzlmod branch from 65b355b to 33cdfa1 Compare February 4, 2025 19:37
@copybara-service copybara-service bot merged commit dde7fb9 into google:main Feb 5, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grebe grebe grebe requested changes

@cdleary cdleary cdleary approved these changes

@mikex-oss mikex-oss mikex-oss approved these changes

@proppy proppy Awaiting requested review from proppy

Assignees
No one assigned
Labels
Reviewing Internally
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hzeller @proppy @cdleary @grebe @mikex-oss