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

Derive jvm_import debug label from explicit attribute #1210

Closed
wants to merge 3 commits into from
Closed

Derive jvm_import debug label from explicit attribute #1210

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9042f39
Derive jvm_import debug label from explicit attribute
plobsing Aug 4, 2024
76a9deb
Also pass user_provided_name in the non-Bzlmod case
plobsing Aug 4, 2024
b0a56fe
Rename workspace_name to repo
plobsing Aug 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 25 additions & 3 deletions private/dependency_tree_parser.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,29 @@ genrule(
target_import_string.append("\tvisibility = [%s]," % (",".join(["\"%s\"" % t for t in target_visibilities])))
alias_visibility = "\tvisibility = [%s],\n" % (",".join(["\"%s\"" % t for t in target_visibilities]))

# 9. Finish the java_import rule.
# 9. Add user-friendly repository name to support diagnostics.
# Only supported by jvm_import.
#
# java_import(
# name = "org_hamcrest_hamcrest_library",
# jars = ["https/repo1.maven.org/maven2/org/hamcrest/hamcrest-library/1.3/hamcrest-library-1.3.jar"],
# srcjar = "https/repo1.maven.org/maven2/org/hamcrest/hamcrest-library/1.3/hamcrest-library-1.3-sources.jar",
# deps = [
# ":org_hamcrest_hamcrest_core",
# ],
# tags = [
# "maven_coordinates=org.hamcrest:hamcrest.library:1.3"],
# "maven_url=https://repo1.maven.org/maven/org/hamcrest/hamcrest-core/1.3/hamcrest-core-1.3.jar",
# "maven:compile-only",
# ],
# neverlink = True,
# testonly = True,
# visibility = ["//visibility:public"],
# user_provided_repo_name = "maven",
if import_rule == "jvm_import":
target_import_string.append("\tuser_provided_repo_name = \"%s\"," % repository_ctx.attr.user_provided_name)

# 10. Finish the java_import rule.
#
# java_import(
# name = "org_hamcrest_hamcrest_library",
Expand All @@ -309,7 +331,7 @@ genrule(

to_return.append("\n".join(target_import_string))

# 10. Create a versionless alias target
# 11. Create a versionless alias target
#
# alias(
# name = "org_hamcrest_hamcrest_library_1_3",
Expand All @@ -335,7 +357,7 @@ processor_class = "{processor_class}",
),
)

# 11. If using maven_install.json, use a genrule to copy the file from the http_file
# 12. If using maven_install.json, use a genrule to copy the file from the http_file
# repository into this repository.
#
# genrule(
Expand Down
4 changes: 2 additions & 2 deletions private/rules/coursier.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1336,7 +1336,7 @@ pinned_coursier_fetch = repository_rule(
attrs = {
"_compat_repository": attr.label(default = "//private:compat_repository.bzl"),
"_outdated": attr.label(default = "//private:outdated.sh"),
"user_provided_name": attr.string(),
"user_provided_name": attr.string(mandatory = True),
"resolver": attr.string(doc = "The resolver to use", values = ["coursier", "maven"], default = "coursier"),
"repositories": attr.string_list(), # list of repository objects, each as json
"artifacts": attr.string_list(), # list of artifact objects, each as json
Expand Down Expand Up @@ -1391,7 +1391,7 @@ coursier_fetch = repository_rule(
"_pin": attr.label(default = "//private:pin.sh"),
"_compat_repository": attr.label(default = "//private:compat_repository.bzl"),
"_outdated": attr.label(default = "//private:outdated.sh"),
"user_provided_name": attr.string(),
"user_provided_name": attr.string(mandatory = True),
"repositories": attr.string_list(), # list of repository objects, each as json
"artifacts": attr.string_list(), # list of artifact objects, each as json
"fail_on_missing_checksum": attr.bool(default = True),
Expand Down
10 changes: 5 additions & 5 deletions private/rules/jvm_import.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,10 @@ def _jvm_import_impl(ctx):
if len(ctx.files.jars) != 1:
fail("Please only specify one jar to import in the jars attribute.")

# With `bzlmod` enabled, workspace names end up being `~` separated. For the
# user-visible workspace name, we need the final part of the name
visible_name = ctx.label.workspace_name.rpartition("~")[2]
label = "@{workspace_name}//{package}:{name}".format(
label = "@{repo}//{package}:{name}".format(
name = ctx.label.name,
package = ctx.label.package,
workspace_name = visible_name,
repo = ctx.attr.user_provided_repo_name,
)

injar = ctx.files.jars[0]
Expand Down Expand Up @@ -100,6 +97,9 @@ jvm_import = rule(
"neverlink": attr.bool(
default = False,
),
"user_provided_repo_name": attr.string(
mandatory = True,
),
"_add_jar_manifest_entry": attr.label(
executable = True,
cfg = "exec",
Expand Down
2 changes: 2 additions & 0 deletions private/rules/maven_install.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ def maven_install(
# created from the maven_install.json file in the coursier_fetch
# invocation after this.
name = name if maven_install_json == None else "unpinned_" + name,
user_provided_name = name,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of adding another parameter, why not derive this from pinned_repo_name, and if it's None or not an attr in the case of pinned_coursier_fetch, use 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.

Inside a repo rule,repository_ctx.attr.name is the fully-qualified repo name; it is no longer the shortname passed when declaring the repo. Using that value yields the following test failure:

INFO: From Testing //tests/unit/manifest_stamp:diff_manifest_test:
==================== Test output for //tests/unit/manifest_stamp:diff_manifest_test:
2c2
< Target-Label: @manifest_stamp_testing//:com_google_guava_guava
---
> Target-Label: @_main~maven~manifest_stamp_testing//:com_google_guava_guava
FAIL: files "tests/unit/manifest_stamp/guava_MANIFEST.MF.golden.unix" and "tests/unit/manifest_stamp/guava_MANIFEST.MF" differ.

IIUC, the preferred way to deal with this is to pass a second string-valued attr, that gets the same value as name, at the declaration site; since this attr is user-declared, it doesn't have any platform magic associated with it - it keeps the same value into the repo rule.

So, the way I see it, we have a need for two user-declared attrs: the shortname of this repo, and the shortname of the assocaited pinned repo (in the case of unpinned repos). I could be wrong, and maybe there's some way to get by without yet another attr here, but I couldn't work out how to do it - the magic associated with the name attr interferes with what we're trying to do.

Copy link
Collaborator

Choose a reason for hiding this comment

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

bazelbuild/bazel#20486 is relevant here. I wonder whether the limitation to debug_print might be a problem. We also have several places in the repo where we calculate the "user visible" name from the repo name. We should really gather those up and have a utility method that will do this for us. #1219 is also relevant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Neat. Should we go with that change instead? Happy to close this if we do; I only care about the result.

pinned_repo_name = None if maven_install_json == None else name,
repositories = repositories_json_strings,
artifacts = artifacts_json_strings,
Expand Down Expand Up @@ -155,6 +156,7 @@ def maven_install(
# Create the repository generated from a maven_install.json file.
pinned_coursier_fetch(
name = name,
user_provided_name = name,
resolver = resolver,
repositories = repositories_json_strings,
artifacts = artifacts_json_strings,
Expand Down