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

Add rules_jni 0.3.1 #47

Merged
merged 1 commit into from
Dec 23, 2021
Merged

Add rules_jni 0.3.1 #47

merged 1 commit into from
Dec 23, 2021

Conversation

fmeum
Copy link
Contributor

@fmeum fmeum commented Dec 19, 2021

No description provided.

@@ -0,0 +1,20 @@
module(
name = "fmeum_rules_jni",
Copy link
Member

Choose a reason for hiding this comment

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

this would need to match the module name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the WORKSPACE world, my ruleset will be known as fmeum_rules_jni because it is not part of the bazelbuild org. Is this kind of prefixing still recommended with modules?

Copy link
Member

Choose a reason for hiding this comment

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

My personal opinion is that we should not do the prefixing, but obviously we're still early enough that best practices can change. Either way, the module name in the registry needs to match the module name in the MODULE.bazel file -- so you need to pick one or the other (and update the other files to match, like presubmit.yml).

If you want to switch to "rules_jni" but are worried about backwards compatibility (e.g. your users are already using @fmeum_rules_jni//foo/bar in their BUILD files), they can simply say bazel_dep(name="rules_jni", repo_name="fmeum_rules_jni") in their MODULE.bazel file when they migrate.

Copy link
Contributor Author

@fmeum fmeum Dec 20, 2021

Choose a reason for hiding this comment

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

The problem is that I would like to keep the name as fmeum_rules_jni for WORKSPACE users. However, I run into a subtle issue with this:

To have them be more realistic, I have extracted all tests for the ruleset into a separate repository fmeum_rules_jni_tests, which is itself a module. Since only the root module can use local overrides, only one of these modules can load the other one as a bazel_dep (see here). But I would like to be able to run the tests both from the main and from the test repository, so fmeum_rules_jni references fmeum_rules_jni_tests via a starlarkified local_repository in a module extension instead (see here).

To carry out the rename but keep using the old name in the tests repository so that the tests still pass without --experimental_enable_bzlmod, I would have to add a repo_mapping to this starlarkified_local_repository that makes the main module, which is canonically called rules_jni, visible to the tests module as fmeum_rules_jni. But it seems that repo_mapping is not recognized on a repository rule instantiated in a module extension, with the following error:

ERROR: <builtin>: //external:.install_dev_dependencies.fmeum_rules_jni_tests: no such attribute 'repo_mapping' in 'starlarkified_local_repository' rule

@Wyverald Is this expected? Do you see another way two local modules can reference each other?

Copy link
Member

Choose a reason for hiding this comment

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

The problem is that I would like to keep the name as fmeum_rules_jni for WORKSPACE users.

WORKSPACE users don't see your MODULE.bazel file at all -- in fact the name of the repo is technically completely up to the user in the WORKSPACE world.

But it seems that repo_mapping is not recognized on a repository rule instantiated in a module extension

Indeed, repo_mapping is not directly available in the bzlmod world, because it's "managed" by bzlmod, so to speak. In your setup, fmeum_rules_jni_tests can specify a repo_name for the bazel_dep on fmeum_rules_jni, and fmeum_rules_jni can rename fmeum_rules_jni_tests to something else when it brings it into scope using use_repo (with a keyword argument).

But anyhow, you can actually just use modules and no extensions here: the modules fmeum_rules_jni_tests and fmeum_rules_jni can depend on each other, and fmeum_rules_jni_tests can specify a local_path_override on fmeum_rules_jni.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But anyhow, you can actually just use modules and no extensions here: the modules fmeum_rules_jni_tests and fmeum_rules_jni can depend on each other, and fmeum_rules_jni_tests can specify a local_path_override on fmeum_rules_jni.

That would be great, but I don't understand yet how fmeum_rules_jni can depend on fmeum_rules_jni_tests. If the latter uses local_path_override, it cannot be depended on by other modules since it wouldn't be the root module. Could you explain this point a bit more?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The core problem is that even if fmeum_rules_jni imports a renamed fmeum_rules_jni_tests with use_repo, the latter will always see the former as fmeum_rules_jni with no chance of renaming.

In more general terms: I want to modify the name a non-module dependency of a module uses to refer to the module itself. It does seem to me that without repo_mappings being available, this is not possible.

Copy link
Member

Choose a reason for hiding this comment

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

If the latter uses local_path_override, it cannot be depended on by other modules since it wouldn't be the root module. Could you explain this point a bit more?

Ah, this is tricky indeed... I was thinking that A and B can depend on each other with B having a local_path_override on A, but in that case A can no longer be used as the "main" repo (even though B can). We can also no longer put A or B in the registry, since B has an override, and A depends on something that has an override (although this latter violation can be worked around with a dev-dep, meaning A is still registry-friendly). So my suggestion wouldn't work without some enhancement to Bzlmod (optional overrides? "dev" overrides?).

In more general terms: I want to modify the name a non-module dependency of a module uses to refer to the module itself. It does seem to me that without repo_mappings being available, this is not possible.

I see your point now -- you're absolutely correct. We had entertained the idea of having a "repo_name" attribute on the "module" directive itself, changing the name of the module for itself* only. We dropped the idea because it's very hacky and had limited use, but maybe we should bring it back on the table.

[*] The module itself, and any module extensions it defines -- because repos generated by module extensions inherit repo-scope from the module that defined the extension.

We don't allow repo_mappings for repo rule invocations in extensions because it would often mean two layers of repo mappings. You can try to map "@fmeum_rules_jni" to "@rules_jni", but this mapping needs to be composed with the bzlmod-managed mapping of "@rules_jni" to "@rules_jni.0.3.1". Without a clear need for such special management, we took the conservative measure of disallowing it altogether. We could also entertain re-allowing this.

I'll think about it a bit more about these options (and please let me know your opinion).

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 think that having repo_name on module could be very helpful for migrating large existing rulesets. For example, rules_go contains large patches adding references to its legacy repo name io_bazel_rules_go to many BUILD files in repositories it creates.

I haven't made up my mind yet regarding solutions to the override issue. My first idea was to introduce a flag --overrides_in_deps with the states ERROR (current behavior, default), WARN and IGNORE that would specify the behavior if an override is encountered in a non-root module. But I think that the "right" way to resolve these limitations will be quite coupled to the testing story for modules in the registry, so perhaps we should wait for the discussion on that.

@fmeum
Copy link
Contributor Author

fmeum commented Dec 21, 2021

@Wyverald I made some progress by (at least temporarily) dropping the test runs from the main repository. This allows me to turn my tests into a proper Bazel module that depends on rules_jni with a local_path_override.

However, I run into a weird issue when I actually carry out the rename, see fmeum/rules_jni#37: I get into a state where the build passes with WORKSPACE and passes with MODULE.bazel together with an almost empty WORKSPACE only specifying the name, but fails if both are present unchanged. (I pushed a temporary commit that disables the WORKSPACE clearing that I implemented for more realistic module setup tests).

The error is:

ERROR: Failed to load Starlark extension '//bzlmod:extensions.bzl'.
Cycle in the workspace file detected. This indicates that a repository is used prior to being defined.
The following chain of repository dependencies lead to the missing definition.
 - @fmeum_rules_jni
This could either mean you have to add the '@fmeum_rules_jni' repository with a statement like `http_archive` in your WORKSPACE file (note that transitive dependencies are not added automatically), or move an existing definition earlier in your WORKSPACE file.
ERROR: cycles detected during target parsing

Could you perhaps explain a bit more how WORKSPACE and MODULE.bazel interact when both are present, especially when using repo_name?

@Wyverald
Copy link
Member

How do you reproduce that error? I tried bazel test --config=bzlmod @fmeum_rules_jni_tests//... on the rename-module branch and got some toolchain errors, but no "WORKSPACE cycle" errors.

Could you perhaps explain a bit more how WORKSPACE and MODULE.bazel interact when both are present, especially when using repo_name?

The way it should work, at least, is that MODULE.bazel stuff takes precedence, and then WORKSPACE stuff is added on top of it (without overriding anything). Repos generated by Bzlmod (except the main repo*) should all work with each other as if WORKSPACE didn't exist. Repos in WORKSPACE (whose names don't coincide with any repos in MODULE.bazel, anyway) can see every other repo in WORKSPACE, along with every module in MODULE.bazel (essentially mapping @foo to @foo.1.0.0).

[*] The main repo (corresponding to the root module) is a bit special in that it can see everything that the root module should see (including bazel_deps and use_repos), and additionally whatever's in WORKSPACE.

This rather convoluted logic is our attempt to cover most migration use cases. The code is in RepositoryMappingFunction.

@fmeum
Copy link
Contributor Author

fmeum commented Dec 21, 2021

@Wyverald The branch is correct, but the issue only comes up when building from within the tests module:

  1. cd tests
  2. bazel test //... --config=bzlmod

@Wyverald
Copy link
Member

Wyverald commented Dec 21, 2021

Ah, we found the bug -- the cycle is caused exactly by that [*] above. When we encounter a load statement from another repo in WORKSPACE, we go to RepositoryDelegatorFunction and ask whether that repo is a module with a non-registry override, or a normal module, or a module extension generated repo, or a repo from WORKSPACE. To know whether it's a module extension generated repo, we need to run extensions. But running an extension defined in the main repo requires the repo mapping of the main repo, which due to [*] contains WORKSPACE repos, so we try to evaluate WORKSPACE again.

There are several ways to break up this cycle; we're going with "running an extension defined in the main repo uses a [*]-less repo mapping", if that makes any sense. I'll try to get the fix into HEAD today.

bazel-io pushed a commit to bazelbuild/bazel that referenced this pull request Dec 21, 2021
…SPACE

(#13316)

See new comment in BzlLoadFunction.java for details. bazelbuild/bazel-central-registry#47 (comment) also has a bit more context.

PiperOrigin-RevId: 417633822
bazel-io pushed a commit to bazelbuild/bazel that referenced this pull request Dec 21, 2021
*** Reason for rollback ***

Causes [failures](https://buildkite.com/bazel/google-bazel-presubmit/builds/52620) in Presubmit of unknown commit, itself a Rollback of 78d0131

*** Original change description ***

Bzlmod: When evaluating extensions in the main repo, do not load WORKSPACE

(#13316)

See new comment in BzlLoadFunction.java for details. bazelbuild/bazel-central-registry#47 (comment) also has a bit more context.

PiperOrigin-RevId: 417668153
@fmeum
Copy link
Contributor Author

fmeum commented Dec 22, 2021

Looks like the fix made it in, but got kicked out again: bazelbuild/bazel@7d09b4a

@Wyverald
Copy link
Member

Argh. It was rolled back because of an unrelated change that broke internal stuff. I'll see if I can sort it out today...

bazel-io pushed a commit to bazelbuild/bazel that referenced this pull request Dec 22, 2021
*** Reason for rollback ***

Now that unknown commit has been submitted, we can roll this forward with a small fix.

*** Original change description ***

Automated rollback of commit 13112c0.

*** Reason for rollback ***

Causes [failures](https://buildkite.com/bazel/google-bazel-presubmit/builds/52620) in Presubmit of unknown commit, itself a Rollback of 78d0131

*** Original change description ***

Bzlmod: When evaluating extensions in the main repo, do not load WORKSPACE

(#13316)

See new comment in BzlLoadFunction.java for details.  bazelbuild/bazel-central-registry#47 (comment) also has a bit more context.

***

PiperOrigin-RevId: 417820112
@Wyverald
Copy link
Member

It's back in :)

@meteorcloudy
Copy link
Member

I have changed the presubmit pipeline to use "last_green" Bazel. Can you push some trivial change to re-trigger the presubmit?

@fmeum fmeum marked this pull request as ready for review December 23, 2021 10:25
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 merged commit 9ddad1f into bazelbuild:main Dec 23, 2021
Wyverald added a commit to bazelbuild/bazel that referenced this pull request Jan 13, 2022
*** Reason for rollback ***

Now that unknown commit has been submitted, we can roll this forward with a small fix.

*** Original change description ***

Automated rollback of commit 13112c0.

*** Reason for rollback ***

Causes [failures](https://buildkite.com/bazel/google-bazel-presubmit/builds/52620) in Presubmit of unknown commit, itself a Rollback of 78d0131

*** Original change description ***

Bzlmod: When evaluating extensions in the main repo, do not load WORKSPACE

(#13316)

See new comment in BzlLoadFunction.java for details.  bazelbuild/bazel-central-registry#47 (comment) also has a bit more context.

***

PiperOrigin-RevId: 417820112
avdv added a commit to tweag/rules_sh that referenced this pull request May 10, 2022
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