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

java_binary: No way to provide dynamic manifest file content #2009

Open
davido opened this issue Oct 29, 2016 · 11 comments
Open

java_binary: No way to provide dynamic manifest file content #2009

davido opened this issue Oct 29, 2016 · 11 comments
Labels
not stale Issues or PRs that are inactive but not considered stale P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Rules-Java Issues for Java rules type: feature request

Comments

@davido
Copy link
Contributor

davido commented Oct 29, 2016

There seems to be no way to provide dynamic content, like build info into MANIFEST.MF file in java_binary rule. Only deploy_manifest_lines is supported, where only list of static strings can be provided.

In Buck there is manifest_file attribute in java_binary rule where a label can be provided.

What people normally want to achieve is to add build version into manifest file. This can be done (since Bazel 0.3.2) with --workspace_status_command=./tools/workspace-status.sh option in .bazelrc file and this genrule:

genrule(
    name = "gerrit_plugin_version",
    stamp = 1,
    cmd = ("grep STABLE_BUILD_GIT_LABEL < bazel-out/stable-status.txt | " +
        "cut -d ' ' -f 2 > $@"),
    outs = ['gerrit_plugin_version.txt'],
)

However, unless I'm missing something obvious, there is no way to merge this version into MANIFEST.MF in java_binary rule:

java_binary(
    name = "gerrit_foo_plugin",
    main_class = "Dummy",
    # TODO(davido): Move this part into manifest
    # when Bazel supports manifest file merging
    resources = [":gerrit_plugin_version.txt"],
    runtime_deps = ["@guava//jar"],
    # Only static key value pairs can be added here.
    deploy_manifest_lines = ["bar: baz"],
)

gerrit_plugin_version.txt is included in the root of the plugin JAR:

cat gerrit_plugin_version.txt
5a6f4fd-dirty

With the above rules we have this content of META-INF/MANIFEST.MF:

  $ bazel build gerrit_foo_plugin_deploy.jar
  INFO: Found 1 target...
Target //:gerrit_foo_plugin_deploy.jar up-to-date:
  bazel-bin/gerrit_foo_plugin_deploy.jar
INFO: Elapsed time: 0.551s, Critical Path: 0.46s

  $ jar -xf bazel-bin/gerrit_foo_plugin_deploy.jar META-INF/MANIFEST.MF
  $ cat META-INF/MANIFEST.MF
Manifest-Version: 1.0
bar: baz
Created-By: blaze-singlejar
Main-Class: Dummy

What I would like to be able to have instead, is:

$ cat META-INF/MANIFEST.MF
[...]
  Implementation-Version: 5a6f4fd-dirty
@laszlocsomor
Copy link
Contributor

I also can't find a way to do this, so I declare this a missing feature.

@ulfjack : Can we add support for this feature? Summary: @davido wants to write workspace status data into the META-INF/MANIFEST.MF in a deploy jar. I think we could add a label attribute to java_binary where the user can specify a custom MANIFEST.MF to achieve this. Do you have anything against that?

@laszlocsomor laszlocsomor added type: feature request P3 We're not considering working on this, but happy to review a PR. (No assignee) labels Oct 31, 2016
@ulfjack
Copy link
Contributor

ulfjack commented Oct 31, 2016

Fewer features is better. I think there are two alternatives here:

  1. Since the manifest is a file and building files is what the build system does, we could deprecate deploy_manifest_lines, and instead add an attribute deploy_manifest which takes a single file. However, we'd need to specify how Main-Class and Created-By get set (or not set), plu s any other values (e.g., for coverage). That'd allow the genrule with stamp=1 approach to work. I had a quick look through our internal code base, and people would like to get stamping information into their deploy manifests as well.

  2. We could always write the stamping information into the deploy manifest. However, I'm not sure we can do that for non-deploy jars (I think that's the reason why stamping information is in a text file right now).

@laszlocsomor
Copy link
Contributor

Fewer features is better. I think there are two alternatives here:

+1

  1. Since the manifest is a file and building files is what the build system does, we could deprecate deploy_manifest_lines, and instead add an attribute deploy_manifest which takes a single file. However, we'd need to specify how Main-Class and Created-By get set (or not set), plu s any other values (e.g., for coverage). That'd allow the genrule with stamp=1 approach to work. I had a quick look through our internal code base, and people would like to get stamping information into their deploy manifests as well.

How about adding another attribute combine_manifest = {append|replace}?

  1. We could always write the stamping information into the deploy manifest. However, I'm not sure we can do that for non-deploy jars (I think that's the reason why stamping information is in a text file right now).

I don't know why that matters, could you explain?

@ulfjack
Copy link
Contributor

ulfjack commented Nov 2, 2016

How about adding another attribute combine_manifest = {append|replace}?

I'm not sure what the semantics of those values are. I am not sure that having different modes is necessary, and we need to talk about how we'd add the aforementioned values to the manifest in the first place.

I don't know why that matters, could you explain?

The code in the deploy jar is the same as in the binary, just that the binary consists of many jar files. Ideally, we use the same mechanism in both cases, otherwise we'd need to detect if we're running from a deploy jar or not. I think it's more 'consistent' with how Java generally works to put the stamping information into the manifest instead of a separate .txt file, but it's not clear to me how that'd work for non-deploy jars.

@laszlocsomor
Copy link
Contributor

I'm not sure what the semantics of those values are. I am not sure that having different modes is necessary, and we need to talk about how we'd add the aforementioned values to the manifest in the first place.

My proposal for the semantics is, if the value is append then bazel generates the default MANIFEST.MF with the usual Created-By and Main-Class fields, and appends the user-provided manifest to that, yielding the final manifest that we pack in the jar.
If the value is replace then we don't generate a default manifest but use the user-provided manifest as-is (and maybe check that it contains these fields).

The code in the deploy jar is the same as in the binary, just that the binary consists of many jar files. Ideally, we use the same mechanism in both cases, otherwise we'd need to detect if we're running from a deploy jar or not. I think it's more 'consistent' with how Java generally works to put the stamping information into the manifest instead of a separate .txt file, but it's not clear to me how that'd work for non-deploy jars.

We could pack the same manifest file in both, no? I'm assuming we have control over what goes into the jar file.

lucamilanesio pushed a commit to GerritCodeReview/gerrit that referenced this issue Nov 2, 2016
Extend workspace status script to scan plugins directory and generate
git version for each of them in stable-status.txt file. This can be
used in plugin build rule to include this as plugin version info.

Given that there is no way to dynamically merge manifest file content
in Bazel[1] in java_binary rule, we have two options: either manipulate
the plugin artifact file by appending the generated plugin version in
post java_binary step in the META-INF/MANIFEST.MF file, or add VERSION
resource file in the plugin and route reading plugin version to this
file.

TEST PLAN:

  $ bazel build :gen_version
  $ cat bazel-out/stable-status.txt
  STABLE_BUILD_COMMIT-MESSAGE-LENGTH-VALIDATOR_LABEL v2.13.1-2-g76b9115
  STABLE_BUILD_COOKBOOK-PLUGIN_LABEL v2.13.2-12-g0162fc1
  STABLE_BUILD_DOWNLOAD-COMMANDS_LABEL v2.13.1-4-g6326db6
  STABLE_BUILD_GERRIT_LABEL v2.13.2-1329-g7c5917b-dirty
  [...]

[1] bazelbuild/bazel#2009

Change-Id: Ie900617e918246ebb54080517161021f3030fdab
@ulfjack
Copy link
Contributor

ulfjack commented Dec 1, 2016

It's not clear how the code finds the right manifest in the non-deploy case.

I think we'd need to look at an example or two to figure out what the right semantics should be here.

@or-shachar
Copy link
Contributor

+1 to this issue.
We actually more interested in controlling the content of the manifest in the non-deploy use case. I'm not sure whether I should open a new issue just on that or just to ask to change the title of this one to be more general to all java_rules.

@GinFungYJF
Copy link
Contributor

Any update? Interested in controlling the content of the manifest in the non-deploy use case, too.

@laszlocsomor
Copy link
Contributor

@iirina : What do you think of this feature request?

lucamilanesio pushed a commit to GerritCodeReview/bazlets that referenced this issue May 17, 2018
Bazel stamp error message is very misleading and hard to track down
and understand. A number of prerequisites must be met for the stamping
machinery to work properly.

Bazel's missing feature to dynamically construct MANIFEST.MF file and
pass such file to java_binary rule contributes to this problem: [1].

To rectify, split the stamp information retrieval in its own rule. The
reason for the failure is because bazel is using "set -o pipefail" for
genrules. This particular option sets the exit code of a pipeline to
that of the rightmost command to exit with a non-zero status, or to
zero if all commands of the pipeline exit successfully.

Given the self descriptive rule name: <plugin>__gen_stamp_info it is
clear where to look for, in tools/workspace-status.sh command.

Test Plan:

1. Apply this CL in bazlets repository
2. Clone reviewers plugin and switch to consume bazlets from local
directory
3. Patch tools/workspace-status.sh file and replace this line:
echo STABLE_BUILD_REVIEWERS_LABEL $(rev .)
with:
echo STABLE_BUILD_REVIEW_LABEL $(rev .)
4. Run bazel build :reviewers and notice that correct failing rule is
now reported:

  $ bazel build reviewers
  INFO: Analysed target //:reviewers (0 packages loaded).
  INFO: Found 1 target...
  ERROR: /home/davido/projects/reviewers/BUILD:3:1: Executing genrule //:reviewers__gen_stamp_info failed (Exit 1)
  Target //:reviewers failed to build

[1] bazelbuild/bazel#2009

Change-Id: Ia2ad7238f77cafb89fa822b78c5d658ccedc8d11
lucamilanesio pushed a commit to GerritCodeReview/bazlets that referenced this issue Jun 12, 2018
Bazel stamp error message is very misleading and hard to track down
and understand. A number of prerequisites must be met for the stamping
machinery to work properly.

Bazel's missing feature to dynamically construct MANIFEST.MF file and
pass such file to java_binary rule contributes to this problem: [1].

To rectify, split the stamp information retrieval in its own rule. The
reason for the failure is because bazel is using "set -o pipefail" for
genrules. This particular option sets the exit code of a pipeline to
that of the rightmost command to exit with a non-zero status, or to
zero if all commands of the pipeline exit successfully.

Given the self descriptive rule name: <plugin>__gen_stamp_info it is
clear where to look for, in tools/workspace-status.sh command.

Test Plan:

1. Apply this CL in bazlets repository
2. Clone reviewers plugin and switch to consume bazlets from local
directory
3. Patch tools/workspace-status.sh file and replace this line:
echo STABLE_BUILD_REVIEWERS_LABEL $(rev .)
with:
echo STABLE_BUILD_REVIEW_LABEL $(rev .)
4. Run bazel build :reviewers and notice that correct failing rule is
now reported:

  $ bazel build reviewers
  INFO: Analysed target //:reviewers (0 packages loaded).
  INFO: Found 1 target...
  ERROR: /home/davido/projects/reviewers/BUILD:3:1: Executing genrule //:reviewers__gen_stamp_info failed (Exit 1)
  Target //:reviewers failed to build

[1] bazelbuild/bazel#2009

Change-Id: Ia2ad7238f77cafb89fa822b78c5d658ccedc8d11
(cherry picked from commit 4459b97)
@aiuto aiuto added team-Rules-Java Issues for Java rules and removed category: rules > java labels Aug 26, 2019
@sgowroji
Copy link
Member

Hi there! We're doing a clean up of old issues and will be closing this one. Please reopen if you’d like to discuss anything further. We’ll respond as soon as we have the bandwidth/resources to do so.

@davido
Copy link
Contributor Author

davido commented Apr 27, 2023

@bazelbuild/triage not stale.

@sgowroji sgowroji reopened this Apr 27, 2023
@sgowroji sgowroji added the not stale Issues or PRs that are inactive but not considered stale label Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not stale Issues or PRs that are inactive but not considered stale P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Rules-Java Issues for Java rules type: feature request
Projects
None yet
Development

No branches or pull requests

8 participants