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(bazel): add MODULE.bazel files for bzlmod #30

Merged
merged 1 commit into from
Jun 4, 2024

Conversation

mmorel-35
Copy link
Contributor

Signed-off-by: Matthieu MOREL [email protected]

@CLAassistant
Copy link

CLAassistant commented Mar 13, 2024

CLA assistant check
All committers have signed the CLA.

@mmorel-35 mmorel-35 force-pushed the bzlmod branch 2 times, most recently from f7da0fd to 8d390f0 Compare March 13, 2024 00:29
@mmorel-35 mmorel-35 marked this pull request as draft March 13, 2024 00:30
@mmorel-35 mmorel-35 force-pushed the bzlmod branch 3 times, most recently from 55c4ed7 to 577018a Compare March 13, 2024 06:28
@mmorel-35 mmorel-35 marked this pull request as ready for review March 13, 2024 06:40
@mmorel-35 mmorel-35 force-pushed the bzlmod branch 2 times, most recently from 4d11c6b to c7d4fa2 Compare March 23, 2024 16:06
@Lynskylate Lynskylate requested a review from zyfjeff March 23, 2024 18:19
@Lynskylate
Copy link
Collaborator

Thank you very much for your contribution, but it seems that ci did not pass. I will check the reason why ci failed, but it may take a while.

cc @zyfjeff

@mmorel-35 mmorel-35 marked this pull request as draft March 31, 2024 19:29
@mmorel-35 mmorel-35 marked this pull request as ready for review May 10, 2024 11:39
MODULE.bazel Outdated
repo_name = "com_google_googletest",
)
bazel_dep(
name = "bazel-compile-commands-extractor",
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the name of bazel_dep does not match the definition in the dependent Module, Bazel will throw an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe that the repo_name is here for that

@Lynskylate
Copy link
Collaborator

Lynskylate commented May 16, 2024

I test it on my computer, and it appears that the reason for the build demo failure is due to refresh_compile_commands being defined in the BUILD file. When hessian2-codec as a module import, it can't load the dependency.

ERROR: error loading package '@@hessian2-codec~//': Unable to find package for @@[unknown repo 'hedron_compile_commands' requested from @@hessian2-codec~]//:refresh_compile_commands.bzl: The repository '@@[unknown repo 'hedron_compile_commands' requested from @@hessian2-codec~]' could not be resolved: No repository visible as '@hedron_compile_commands' from repository '@@hessian2-codec~'.

After I removed the refresh_compile_commands in the BUILD file, it can build success.

@mmorel-35 mmorel-35 marked this pull request as draft May 16, 2024 08:09
@mmorel-35
Copy link
Contributor Author

Let's keep this a draft until this bazelbuild/bazel-central-registry#1989 is merged

@Lynskylate
Copy link
Collaborator

Thank you very much.

@mmorel-35 mmorel-35 marked this pull request as ready for review May 17, 2024 06:55
@mmorel-35 mmorel-35 requested a review from Lynskylate May 17, 2024 06:56
@mmorel-35 mmorel-35 marked this pull request as draft May 17, 2024 07:11
@mmorel-35 mmorel-35 marked this pull request as ready for review May 17, 2024 07:21
@mmorel-35
Copy link
Contributor Author

@Lynskylate , that’s ready for your review. There won’t be any change concerning hedron_compile_commands

@Lynskylate
Copy link
Collaborator

Lynskylate commented May 18, 2024

git-override implict do same action as dev_dependency.

This directive only takes effect in the root module; in other words, if a module is used as a dependency by others, its own overrides are ignored.

This is also the reason why it fails to load refresh_compile_commands in the demo.

I am still reading the documentation on dev dependencies to see if I should consider loading that target only when in dev dependency mode.

@mmorel-35
Copy link
Contributor Author

mmorel-35 commented May 18, 2024

I created this : bazelbuild/bazel-central-registry#2054

I believe adding to the BCR is going to become a necessity.

@Lynskylate
Copy link
Collaborator

Lynskylate commented May 18, 2024

I may not have been clear.
Because hessian2-codec, as a module, depends on hedron_compile_commandsand also uses git_override to override this module. Therefore, when the demo module depends on the hessian2-codec module, the demo cannot find the hedron_compile_commandsand that the Hessian module depends on.

So if we want to fix the error, Perhaps we could first simply remove the refresh_compile from the hessian2-codec BUILD.

@Lynskylate
Copy link
Collaborator

And i have test the different version bazel for the build, it can build in bazel 6.0 but can't work in bazel 7.0+.

@mmorel-35
Copy link
Contributor Author

Perhaps we could first simply remove the refresh_compile from the hessian2-codec BUILD.

It can be removed if you don't need it but if bazelbuild/bazel-central-registry#2055 get merged we shall be able to make things works.

@mmorel-35
Copy link
Contributor Author

mmorel-35 commented May 29, 2024

@Lynskylate, can you retry?
This follows @cpsauer's preconisation

@Lynskylate
Copy link
Collaborator

I apologize for not responding promptly. I have tested before, and there might be some issues with calculating coverage after migrating to the module. We can merge this PR first, and then I will create another PR to fix this issue.

@Lynskylate
Copy link
Collaborator

@zyfjeff @wbpcode
If you have no objections, I will merge this pull request first, and then create another pull request later to fix the issues.

The current compilation parameters under the demo will cause a failure, and in addition to this, there is also an incompatibility with coverage.

You could see the action detail.
https://github.com/Lynskylate/hessian2-codec/actions/runs/9139284705/job/25131395468

@Lynskylate Lynskylate merged commit 05e2938 into alibaba:main Jun 4, 2024
1 of 2 checks passed
@Lynskylate
Copy link
Collaborator

@mmorel-35 Thanks for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants