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

$(location) should use runtime path when used in jvm_flags attribute #2475

Closed
shahms opened this Issue Feb 2, 2017 · 88 comments

Comments

@shahms

shahms commented Feb 2, 2017

We have a binary which requires the javac.jar be present in its bootclasspath. Prior to bazel 0.4.4, the following invocation worked (but was obviously less than ideal):

jvm_flags = [
    "-Xbootclasspath/p:$$JAVA_RUNFILES/bazel_tools/third_party/java/jdk/langtools/javac.jar",
],

With bazel 0.4.4 the file name specific path has broken, as the actual file name of the target is now javac-9-dev-r3297-1.jar.

Ideally, we'd be able to use $(location @bazel_tools//third_party/java/jdk/langtools:javac_jar), however that expands to external/bazel_tools/third_party/java/jdk/langtools/javac-9-dev-r3297-1.jar which is the build path and not the runtime path for that file. Needless to say, I'd like to avoid hard-coding another path which will break or fragile inline path-munging hacks. It seems like the correct answer is for $(location) to use the runtime path when specified in jvm_flags.

@meteorcloudy

This comment has been minimized.

Member

meteorcloudy commented Feb 3, 2017

@damienmg Do you think it's reasonable to support this feature? Or is there a better way to find the runtime path of javac.jar?

@meteorcloudy

This comment has been minimized.

Member

meteorcloudy commented Feb 3, 2017

//cc @kchodorow

@damienmg

This comment has been minimized.

Contributor

damienmg commented Feb 3, 2017

FWIW we cannot make $(location ) returns an absolute path but it should be able to return a path relative to the runfile tree. @kchodorow should definitely knows how / when it will be possible to do so.

@kchodorow

This comment has been minimized.

Contributor

kchodorow commented Apr 24, 2017

It's an awfully big hammer, but Skylark's short_path gives you the runtime path. We're working on making the runtime path match the build path (for sources, at least), but it's taking a lot longer than planned.

@ittaiz

This comment has been minimized.

Member

ittaiz commented May 17, 2017

Hey, I have this exact same need but I can't use short_path since we're talking about a macro...
It's basically wrapping rules_scala with a macro and computing some values (one of them being the short_path of the output of the rule being wrapped)

@ittaiz

This comment has been minimized.

Member

ittaiz commented May 22, 2017

ping. I need this to be able to work with sandboxing/remote execution...

@mhlopko

This comment has been minimized.

Contributor

mhlopko commented May 23, 2017

Adding a link to a corresponding StackOverflow question.

@ulfjack

This comment has been minimized.

Contributor

ulfjack commented May 23, 2017

It seems reasonable to change $(location) to output the runtime path for attributes that are clearly only used at runtime. If there are any cases where it's not clear, we could add another function $(rlocation) or $(runtime_location) to return the runtime path.

@ittaiz

This comment has been minimized.

Member

ittaiz commented May 23, 2017

Another function would be great!

@ulfjack

This comment has been minimized.

Contributor

ulfjack commented May 23, 2017

Do you think that there are a lot of cases where it's unclear?

@ittaiz

This comment has been minimized.

Member

ittaiz commented May 23, 2017

Not at all. I have two use cases which both boil down to jvm_flags (one BUILD file and one in a macro). I'm just afraid a bit that changing the semantics would take longer than adding a new function.

@ulfjack

This comment has been minimized.

Contributor

ulfjack commented May 24, 2017

I can think of use cases that require making this configurable by the user (genrule.cmd), but maybe we should also disallow $(location) for jvm_flags, where it's basically never correct (I think).

@ittaiz

This comment has been minimized.

Member

ittaiz commented May 24, 2017

@shahms

This comment has been minimized.

shahms commented May 24, 2017

Banning $(location) in jvm_flags seems like the opposite of a solution.

@ulfjack

This comment has been minimized.

Contributor

ulfjack commented May 24, 2017

jvm_flags are only applied at runtime, so a function that expands to the compile-time path is 'never' correct, except it happens to be ok for source files.

@ulfjack

This comment has been minimized.

Contributor

ulfjack commented May 24, 2017

@shahms - the proposal is to add $(rlocation), replace all uses of $(location) in jvm_flags with $(rlocation), and then disallow using $(location) in jvm_flags (ideally with a good error message).

@shahms

This comment has been minimized.

shahms commented May 24, 2017

That makes more sense, thanks :-)

@lberki

This comment has been minimized.

Contributor

lberki commented Jun 14, 2017

See b1b02d7 for prior art.

@buchgr

This comment has been minimized.

Contributor

buchgr commented Jun 14, 2017

Our documentation seems to suggest that file paths returned by $(location ...) are runtime paths

The expanded paths are relative to the runfiles directory of the *_test or *_binary rule.

https://bazel.build/versions/master/docs/be/make-variables.html#location

@lberki

This comment has been minimized.

Contributor

lberki commented Jun 14, 2017

That's untrue at least for genrules. A quick glance at the code says that we get execpaths, which makes sense because otherwise the $(location) support in java_toolchain wouldn't make sense.

@ulfjack

This comment has been minimized.

Contributor

ulfjack commented Jun 14, 2017

@lberki

This comment has been minimized.

Contributor

lberki commented Jun 14, 2017

Yeah. Not from the BUILD file, though.

@ulfjack

This comment has been minimized.

Contributor

ulfjack commented Jun 14, 2017

@buchgr just found out that this bug report is a bit vague in that it doesn't mention which rule you're using. java_binary actually expands the runtime location, not the compile-time location in jvm_flags, but scala_binary doesn't. The ctx.expand_location Skylark method calls into the code path that expands the compile-time location, and is not configurable. Unfortunately, if we need a Skylark API change, then this becomes more complex to implement. :-/

bazel-io pushed a commit that referenced this issue Oct 2, 2017

Make JavaToolchain use the new Expander API
Progress on #2475.

PiperOrigin-RevId: 170671644

bazel-io pushed a commit that referenced this issue Oct 2, 2017

LocationExpander: always require options to be passed in
Also update CommandHelper to split the heuristic label expansion code path from
the normal code path.

Progress on #2475.

PiperOrigin-RevId: 170675702

bazel-io pushed a commit that referenced this issue Oct 2, 2017

Rewrite LocationExpander
Split up the functionality into separate classes, and test each independently.
(Keep one integration test to make sure it still works together.)

This is in preparation for adding another location function for runfiles paths.
Currently we have to decide ahead of time whether to expand artifacts as exec
paths or root-relative (runfiles) paths, but there are cases where we can't
make that decision ahead of time and / or need both to coexist, even in the
same attribute.

Progress on #2475.

PiperOrigin-RevId: 170691666
@ulfjack

This comment has been minimized.

Contributor

ulfjack commented Oct 10, 2017

The changes above are part of an internal refactoring that should make it much more straightforward to define and implement the right Skylark API. However, I haven't even started on the Skylark API yet, and that also needs to be reviewed by the Skylark team. I think that I'll have a Skylark API proposal in the next two weeks, plus some time for reviewing, and then some more time for implementation.

I still have three (3) pending changes at this point as part of the refactoring.

@ittaiz

This comment has been minimized.

Member

ittaiz commented Oct 10, 2017

@ulfjack

This comment has been minimized.

Contributor

ulfjack commented Oct 10, 2017

Uhm. If you are relying on a binary that you configure with a file path, and you're using $(location) for the file path, and you are trying to run that binary in an action, and the binary is executed in the Bazel exec root, then the action will likely fail if executed remotely. You can still use remote execution for all other actions, and you can use remote caching in any case.

You can annotate actions / rules with the "local" tag to get them to not be executed remotely (although I think if you do then they're also not remotely cached).

@ittaiz

This comment has been minimized.

Member

ittaiz commented Oct 10, 2017

@ulfjack

This comment has been minimized.

Contributor

ulfjack commented Oct 23, 2017

Actually, I have a plan for implementing the new rlocation (exact name TBD) function without changing the Skylark API. It would be nicer to have a new Skylark API, but it doesn't necessarily block fixing this in the short term. I should have an update this week.

@ulfjack

This comment has been minimized.

Contributor

ulfjack commented Oct 25, 2017

My proposal is that we add $(execpath), $(execpaths), $(rootpath), and $(rootpaths) functions that work the same way as $(location), but always use exec paths or root-relative paths (runtime / runfiles paths) respectively. The way I'm adding them they become available whenever $(location) is available, and perform the exact same label / dependency lookup.

I have a pending change for this, on top of a stack of four more pending changes to finish the internal refactorings.

@ittaiz

This comment has been minimized.

Member

ittaiz commented Oct 25, 2017

Excuse me for the noob question but what is the different between execpath and rootpath? Or rather why is rootpath needed? I assume execpath is like $(location) but when I execute the code

@ulfjack

This comment has been minimized.

Contributor

ulfjack commented Oct 25, 2017

For example, say you have expansion/BUILD:

genrule(name = "foo", outs = ["foo.txt"], srcs = ["bar.txt"], cmd = "cp $< $@")

foo.txt:
execpath = bazel-out/darwin-fastbuild/bin/expansion/foo.txt
rootpath = expansion/foo.txt

bar.txt:
execpath = expansion/bar.txt
rootpath = expansion/bar.txt

The current $(location) function sometimes returns the exec path, and sometimes returns the root-relative path, depending on how the expander is configured (except you can't configure it through the skylark API, which has no knob for that).

@ittaiz

This comment has been minimized.

Member

ittaiz commented Oct 25, 2017

@ulfjack

This comment has been minimized.

Contributor

ulfjack commented Nov 2, 2017

Unfortunately there have been some delays due to travel and public holidays in Germany, as well as a commit gone wrong, which I had to revert. I'm currently rolling it forward again, and I have a few more changes on top of that, but they're not under review yet. It looks like we'll miss the 0.8.0 window, unfortunately.

bazel-io pushed a commit that referenced this issue Nov 3, 2017

Add rootpath(s) and execpath(s) functions to template expansion
In addition to the $(location) function, we now also support $(rootpath) and
$(execpath) functions.

Unfortunately, we have to do this in two places since the Skylark API for expand_location has to continue calling into LocationExpander in order to preserve its semantic contract.

Progress on #2475.

RELNOTES[NEW]:
    In addition to $(location), Bazel now also supports $(rootpath) to obtain
    the root-relative path (i.e., for runfiles locations), and $(execpath) to
    obtain the exec path (i.e., for build-time locations)

PiperOrigin-RevId: 174454119
@alexeagle

This comment has been minimized.

alexeagle commented Nov 10, 2017

When this does land - please also add testing for cross-workspace paths. If I use $(rootpath @other_wksp//:thing) I would expect external/other_wksp/thing or so

@davido

This comment has been minimized.

Contributor

davido commented Nov 25, 2017

Exception in thread "main" java.lang.NoSuchFieldError: ANNOTATION_PROCESSOR_MODULE_PATH

I'm seeing a similar failure when running Bazel java_tools tests on Windows: #4165. All is fine on Linux, though.

bazel-io pushed a commit that referenced this issue Dec 18, 2017

Change CommandHelper to use TemplateExpander directly
This is a partial rollback of e8d32b7, only the CommandHelper change.

Progress on #2475.

PiperOrigin-RevId: 179413908

bazel-io pushed a commit that referenced this issue Dec 20, 2017

Automated rollback of commit 2f10da0.
*** Reason for rollback ***

Breaks skylark rules using $ in action cmd line.

*** Original change description ***

Change CommandHelper to use TemplateExpander directly

This is a partial rollback of e8d32b7, only the CommandHelper change.

Progress on #2475.

PiperOrigin-RevId: 179607027
@rzhw

This comment has been minimized.

Contributor

rzhw commented Feb 22, 2018

Quick note that cross-workspace path tests were added in 702429b.

@ulfjack ulfjack closed this Jun 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment