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

Windows support #717

Closed
wants to merge 2 commits into from
Closed

Conversation

majcherm-da
Copy link
Contributor

This resolves 2 issues I raised for running rules_scala on Windows, that is:

Main change is a port of Bazel's exe launcher from java rules to scala rules. Without it Bazel attempts to execute a bash script (generated from java_stub_template) as Bazel's executable, which fails on Windows.

See https://github.com/bazelbuild/bazel/blob/8ac46c157c5da63e97a5825316ce6f0c3290e189/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaSemantics.java#L411 for how it's done in java rules.

The UnusedDependencyChecker is fixed by normalizing Windows paths, replacing backslashes with slashes. Without it all targets report unused deps on Windows.

@majcherm-da majcherm-da requested a review from johnynek as a code owner March 19, 2019 13:45
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@majcherm-da
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@ittaiz
Copy link
Member

ittaiz commented Mar 26, 2019

@majcherm-da Thank you and sorry for the long silence. I'll get to this very soon.
I've skimmed and just want to verify something- the "exe" launcher which is a java_binary is used only in windows, right?
Also this starts a jvm which then starts a windows executable from core-bazel?
Is this indirection mandatory? Sounds like it's added complexity and performance but I don't know the windows challenges well enough.

@ittaiz
Copy link
Member

ittaiz commented Mar 26, 2019

Also,
@johnynek I think this relates to the discussion we had about dropping scala_binary and its friends in favor of java_binary and friends.
We are making some headway at Wix towards being able to move from scala_junit_test to using java_test (still not there but I think we're close).
If we can run scala_test on java_test/java_binary then we can remove a lot of code/concerns from this repo.

Copy link
Member

@johnynek johnynek left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

Is there any way we can get CI testing Windows so we don’t break this going forward?

If this is too much pain for the PR I’m in favor of a merge and then follow up later, but I guess we’ll break it soon without CI.

@majcherm-da
Copy link
Contributor Author

@ittaiz Yes, the "exe" java_binary is used only on Windows. It's java based, since it was the simpliest way I found to create the resulting exe file based on the Bazel's bazel_tools/tools/launcher/launcher.exe (porting the solution from Bazel's JavaBinary). The launcher.exe (binary file) needs to be appended with target specific arguments, which together constitues a final exe file. Bazel's JavaBinary is written in Java itself, but here the entrypoint is a Skylark rule that's why it feels a bit unnatrual. The "exe" java_binary is only used to create the final exe file, which is a replacement for the sh start script for Linux/Mac to start the target app/binary, so the statement that "this starts a jvm which then starts a windows executable from core-bazel" is not fully correct. As an alternative, to get rid of the need of starting JVM to create the exe file, we could re-implement the Bazel's LaunchInfo and a merge of it's output with the laucher.exe binary with sth. more lightweight, like a run_shell call, but, not mentioning the rewrite itself, it might be a bit tricky to do in bash as it would need to support the runfiles.rlocation to obtain the path of the launcher.exe.

@johnynek
Copy link
Member

can we close this since #718 includes everything here?

@johnynek johnynek mentioned this pull request Aug 19, 2019
gergelyfabian pushed a commit to gergelyfabian/rules_scala that referenced this pull request May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants