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

Son of "Bad Path Elements" (the next generation) #1374

Open
virusdave opened this issue Mar 29, 2022 · 8 comments
Open

Son of "Bad Path Elements" (the next generation) #1374

virusdave opened this issue Mar 29, 2022 · 8 comments

Comments

@virusdave
Copy link
Contributor

Issue #1257 is happening still with modern bazel and rules_scala.

$ bazel build  @io_bazel_rules_scala//src/java/io/bazel/rulesscala/scalac:scalac.jar   2>&1 | $PAGER

──────────────────────────────────────────────────────────────────────────
       │ STDIN
──────────────────────────────────────────────────────────────────────────
   1   │ INFO: Invocation ID: de2a58ee-4a26-428c-b775-8990ef9c6017
   2   │ Loading:
   3   │ Loading: 0 packages loaded
   4   │ Analyzing: target @io_bazel_rules_scala//src/java/io/bazel/rulesscala/scalac:scalac.jar (1 packages loaded, 0 targets configured)
   5   │ INFO: Analyzed target @io_bazel_rules_scala//src/java/io/bazel/rulesscala/scalac:scalac.jar (1 packages loaded, 7 targets configured).
   6   │ INFO: Found 1 target...
   7   │ [0 / 2] [Prepa] BazelWorkspaceStatusAction stable-status.txt
   8   │ INFO: From Building external/io_bazel_rules_scala/src/java/io/bazel/rulesscala/scalac/scalac.jar (4 source files):
   9   │ warning: [path] bad path element "/private/var/tmp/_bazel_dave/f3114e5a271e277eec60625145f8bcd1/execroot/rh/bazel-out/darwin-fastbuild/bin/external/io_bazel_rules_scala_scala_compiler/io_bazel_rules_scala_scala_compiler.stamp/scala-
       │ reflect.jar": no such file or directory
  10   │ warning: [path] bad path element "/private/var/tmp/_bazel_dave/f3114e5a271e277eec60625145f8bcd1/execroot/rh/bazel-out/darwin-fastbuild/bin/external/io_bazel_rules_scala_scala_compiler/io_bazel_rules_scala_scala_compiler.stamp/scala-
       │ library.jar": no such file or directory
  11   │ Target @io_bazel_rules_scala//src/java/io/bazel/rulesscala/scalac:scalac.jar up-to-date:
  12   │   bazel-bin/external/io_bazel_rules_scala/src/java/io/bazel/rulesscala/scalac/scalac.jar
  13   │ INFO: Elapsed time: 0.292s, Critical Path: 0.01s
  14   │ INFO: 2 processes: 1 disk cache hit, 1 internal.
  15   │ INFO: Build completed successfully, 2 total actions
  16   │ INFO: Build completed successfully, 2 total actions

It was discussed in this upstream bazel thread
rules_jvm_external solves this problem by stripping the classpath elements from the manifest of jars it imports.

It looks like rules_scala uses its own custom rule implementation to import its dependencies (such as scala-compiler-2.12.14.jar, which has this manifest entry set). We probably need to do something similar right here, either using rules_jvm_external's tool directly (if allowed), or having a similar bespoke implementation which does this if a dependency on rules_jvm_external from these rules_scala is not allowed.

In the meantime, i'm going to try to solve this locally by replacing the rules dependencies with jars which have already passed through the rules_jvm_external manifest rewriting, since we already do use that ruleset. Will report back if this hack works to prevent logspam.

@liucijus
Copy link
Collaborator

This seems to be a problem within ijar. There was a fix #1258 by @simuons, maybe it can be upstreamed to Bazel?

I assume, that users of Rules Scala use custom loaders like rules_jvm_external, which can solve this problem, so this is mostly a problem within rules_scala repo, no?

@virusdave
Copy link
Contributor Author

I've only so far seen this warning output on the jars imported directly in rules_scala via its internal import mechanism. rules_jvm_external (modern versions) seem to handle this problem just fine in the "general jar" case (aside from signed jars, i imagine)

@virusdave
Copy link
Contributor Author

virusdave commented Mar 29, 2022

I'm curious why you say

This seems to be a problem within ijar.

though. I didn't see anywhere in rules_scala where there manually-imported (ie, scala_imported) jars are being passed through ijar.

Until recently, rules_scala used a custom ijar to do manifest stamping, but now it just uses the standard java tools to do that. #1258 updated the custom tool (which was previously used for this) to also remove the classpath elements, but neither of those functionalities (stamping, manifest update) are really the point of ijar, right?

@liucijus
Copy link
Collaborator

java_common.stamp_jar uses ijar (I believe it was cc binary on the default JavaToolchain last time I checked) to do its job here.

@long-stripe
Copy link
Contributor

I looked into as well and I seem to think that this chunder should be fixed in Bazel by bazelbuild/bazel@5ea498c, which hasn't been released yet. Maybe in Bazel 6?

See also:

@virusdave
Copy link
Contributor Author

virusdave commented Mar 29, 2022

I looked into as well and I seem to think that this chunder should be fixed in Bazel by bazelbuild/bazel@5ea498c, which hasn't been released yet. Maybe in Bazel 6?

I'm using a pre-release build of bazel 6 which includes this commit, and it does not solve the problem.

In the meantime, i'm going to try to solve this locally by replacing the rules dependencies with jars which have already passed through the rules_jvm_external manifest rewriting, since we already do use that ruleset. Will report back if this hack works to prevent logspam.

Indeed, running the set of repository jars through rules_jvm_external's jvm_import(), then using those filtered output jars// in custom classpath providers in our custom toolchain rule does indeed solve the bad path elements problem as expected.

This works fine because we already use rules_jvm_external in our repository, so it's readily available to us. IF we'd be willing to add that as a required dependency to rules_scala, then i'd be happy to upstream the patch to make use of it to solve the issue globally. If we're not willing to add that dependency, then we'd need something similar to what I added

I'll move on to the scalapb deprecations build spam next.

@johnynek
Copy link
Member

just want to chime in: I (and others) still use https://github.com/johnynek/bazel-deps/ which does not depend on rules_jvm_external (yet?) so I kind of hope we don't require that to set up rules_scala.

One hallmark of monorepos and bazel installs is that they are generally highly customized. Putting minimal dependencies on folks is a big plus.

@virusdave
Copy link
Contributor Author

just want to chime in: I (and others) still use https://github.com/johnynek/bazel-deps/ which does not depend on rules_jvm_external (yet?) so I kind of hope we don't require that to set up rules_scala.

One hallmark of monorepos and bazel installs is that they are generally highly customized. Putting minimal dependencies on folks is a big plus.

Yup, makes sense. If we don't want that dependency edge, then i think we need a custom implementation in these rules to do manifest classpath scrubbing like rules_jvm_external does (at least until this is solved upstream in bazel and exposed via a java_common rule/macro) :(

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

No branches or pull requests

4 participants