[singlejar] Port test_util to Windows #6248

rongjiecomputer · 2018-09-26T07:56:32Z

Port various functions in test_util to Windows.
Replace @local_jdk//:jdk-default with @bazel_tools//tools/jdk:current_java_runtime.

rongjiecomputer · 2018-09-26T08:05:30Z

meteorcloudy · 2018-09-26T13:18:39Z

I've imported this change, but the C++ runfiles library doesn't work well internally. I need some time to debug and fix the issue. Sorry for the delay.

laszlocsomor · 2018-09-27T07:33:56Z

I'll take a look at why the runfiles library is failing in the Google-internal code base.

src/tools/singlejar/BUILD

laszlocsomor · 2018-09-27T12:32:46Z

src/tools/singlejar/test_util.cc

+int main(int argc, char** argv) {
+  std::string error;
+  std::unique_ptr<singlejar_test_util::Runfiles> runfiles(
+      singlejar_test_util::Runfiles::Create(argv[0], &error));


Unfortunately I can't fix the error without patching the runfiles library, which I'm about to do. Could you please hold this PR until that one is merged?

My patch will, among other things, add a static Runfiles* Runfiles::CreateForTest(void) method, which won't require argv0. Therefore there'll be no need for a custom main method.

Once my patch is in, could you please init the runfiles either on-demand (in tests that need it) or in the setUp method, using the upcoming CreateForTest factory method?

Could you please hold this PR until that one is merged?

Sure thing. Ping me when that PR is merged. #6251 probably will need some changes too, so don't review that one yet.

laszlocsomor · 2018-09-27T12:35:51Z

src/tools/singlejar/test_util.cc

@@ -114,4 +135,19 @@ string CreateTextFile(const string& relpath, const char *contents) {
  return string("");
 }

+Runfiles* runfiles = nullptr;


Is there a way to not use a global variable? (Maybe by initializing runfiles in the setUp method?)

Initializing runfiles in the setUp is fine. I used global variable when I started porting tests out of convenience.

laszlocsomor · 2018-09-28T08:24:19Z

@rongjiecomputer : Thank you so much for waiting! #6267 is now merged.

rongjiecomputer · 2018-10-01T12:35:13Z

@laszlocsomor This PR is ready now.

laszlocsomor · 2018-10-01T12:40:19Z

Thanks @rongjiecomputer ! I don't see the code using the runfiles library anywhere. Did you mean to use it, or just adding the dependency?

rongjiecomputer · 2018-10-01T12:42:02Z

That will be in #6251, which I have not yet updated.

laszlocsomor · 2018-10-01T13:41:57Z

src/tools/singlejar/test_util.cc

@@ -87,7 +108,7 @@ string GetEntryContents(const string &zip_path, const string &entry_name) {
  string command;
  blaze_util::StringPrintf(&command, "unzip -p %s %s", zip_path.c_str(),
                           entry_name.c_str());
-  FILE *fp = popen(command.c_str(), "r");
+  FILE *fp = popen(command.c_str(), "rb");


Unfortunately this change breaks the internal version of output_jar_simple_test.
Is this change necessary?

http://man7.org/linux/man-pages/man3/popen.3.html doesn't document b; it seems to be Microsoft-specific.

(https://msdn.microsoft.com/en-us/library/96ayss4b.aspx)

The "b" is necessary because otherwise the Microsoft implementation will strip \r\n to \n and make many tests fail. I will add a _WIN32 guard for this.

Thanks!
With your permission, I'd like to also add this comment:

// "b" is Microsoft-specific. It's necessary, because without it the popen // implementation will strip "\r\n" to "\n" and make many tests fail.

(If you agree to it, just let me know, no need to upload a new commit.)

You have my permission.

laszlocsomor · 2018-10-02T13:25:17Z

I haven't forgotten about this! Trying to merge now, tests are still running.

ulfjack · 2018-10-11T03:15:33Z

src/tools/singlejar/BUILD

@@ -232,12 +232,13 @@ cc_test(
        ":test1",
        ":test2",
        "@local_jdk//:jar",
-        "@local_jdk//:jdk-default",
+        "@local_jdk//:jdk",


Ideally, this would be fixed not to use @local_jdk. Also, did this test not actually run before? There's no :jdk-default target, I think.

Ideally, this would be fixed not to use @local_jdk

So what is the alternative?

I also have been wondering whether some of the singlejar tests were even tested before, and whether the //src/tools/singlejar/BUILD in Bazel is different from the Google internal one (because some test files are never included in the BUILD file and see this comment: #6336 (comment)).

I believe //external:jdk and //external:jar would be more desirable. Alas, I'm not at all familiar with these JDK rules. I'm consulting with @lberki for advice on the internal review.

Me! If all the test needs is to run jar, do this instead:

cc_test( name="test", data=["@bazel_tols//tools/jdk:current_java_runtime"], toolchains=["@bazel_tools//tools/jdk:current_java_runtime"], copts=["-DJAR=$(JAVA)/bin/jar"], )

This way, you get a path to whatever target JDK is used (and not to the result of autodetection), which is the principled thing to do.

I think we need to add .exe at for the -D flag. Windows also requires different kind of " escaping to pass string constant in -D as const char[]. I will handle that in the second PR, just do whatever that works for Unix first.

Sounds like a plan. You can use select({"@bazel_tools//platforms:windows": ... }) or something like that to handle that case.

@rongjiecomputer : shall I apply @lberki 's suggestion (the cc_test rule) to the code and submit internally like so?

I introduced these problems while editing the Google-internal copy of bazelbuild#6248 that was later pushed as bazelbuild@68611b3. See bazelbuild#2241 Change-Id: Icd981a0fcdcd451a00cbb2babd107f7abb4d5170

I introduced these problems while editing the Google-internal copy of #6248 that was later pushed as 68611b3. See #2241 Change-Id: Icd981a0fcdcd451a00cbb2babd107f7abb4d5170 Closes #6404. Change-Id: I33cc9c5890ec5236e7964846c158628c947c715d PiperOrigin-RevId: 217304415

googlebot added the cla: yes label Sep 26, 2018

iirina requested review from meteorcloudy and laszlocsomor September 26, 2018 08:11

iirina added the area-Windows Windows-specific issues and feature requests label Sep 26, 2018

rongjiecomputer mentioned this pull request Sep 26, 2018

[singlejar] Use Bazel runfiles lib in test and other Windows fixes #6251

Closed

meteorcloudy approved these changes Sep 26, 2018

View reviewed changes

laszlocsomor suggested changes Sep 27, 2018

View reviewed changes

laszlocsomor reviewed Sep 27, 2018

View reviewed changes

rongjiecomputer force-pushed the test_util branch from 9e62038 to b2fe671 Compare September 28, 2018 08:53

rongjiecomputer added 4 commits October 1, 2018 20:39

[singlejar] Port test_util for Windows

1fe4226

Remove *runfiles, custom int main and use gtest_main

4f4d6f5

Remove unused include

dcdf7f7

Use //tools/cpp/runfiles temporarily

e48a618

rongjiecomputer force-pushed the test_util branch from a483cc7 to e48a618 Compare October 1, 2018 12:40

laszlocsomor approved these changes Oct 1, 2018

View reviewed changes

laszlocsomor reviewed Oct 1, 2018

View reviewed changes

Use popen(f, rb) for Windows only and add testonly=1 for test_util

af99366

This was referenced Oct 4, 2018

[singlejar] WIP: Porting tests for Windows #6031

Closed

Use the new singlejar on Windows #2241

Closed

ulfjack reviewed Oct 11, 2018

View reviewed changes

bazel-io closed this in 68611b3 Oct 15, 2018

rongjiecomputer deleted the test_util branch October 15, 2018 14:44

laszlocsomor mentioned this pull request Oct 16, 2018

Singlejar: fix bad use of $(JAVA) and $(locations) #6404

Closed

philwo added the team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website label Jun 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[singlejar] Port test_util to Windows #6248

[singlejar] Port test_util to Windows #6248

rongjiecomputer commented Sep 26, 2018 •

edited

Loading

rongjiecomputer commented Sep 26, 2018

meteorcloudy commented Sep 26, 2018

laszlocsomor commented Sep 27, 2018

laszlocsomor Sep 27, 2018

rongjiecomputer Sep 27, 2018

laszlocsomor Sep 27, 2018

rongjiecomputer Sep 27, 2018 •

edited

Loading

laszlocsomor commented Sep 28, 2018

rongjiecomputer commented Oct 1, 2018

laszlocsomor commented Oct 1, 2018

rongjiecomputer commented Oct 1, 2018

laszlocsomor Oct 1, 2018

laszlocsomor Oct 1, 2018

laszlocsomor Oct 1, 2018

rongjiecomputer Oct 1, 2018

laszlocsomor Oct 1, 2018

rongjiecomputer Oct 1, 2018

laszlocsomor commented Oct 2, 2018

ulfjack Oct 11, 2018

rongjiecomputer Oct 11, 2018 •

edited

Loading

laszlocsomor Oct 11, 2018

lberki Oct 11, 2018

rongjiecomputer Oct 11, 2018

lberki Oct 11, 2018

laszlocsomor Oct 15, 2018

rongjiecomputer Oct 15, 2018

[singlejar] Port test_util to Windows #6248

[singlejar] Port test_util to Windows #6248

Conversation

rongjiecomputer commented Sep 26, 2018 • edited Loading

rongjiecomputer commented Sep 26, 2018

meteorcloudy commented Sep 26, 2018

laszlocsomor commented Sep 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rongjiecomputer Sep 27, 2018 • edited Loading

Choose a reason for hiding this comment

laszlocsomor commented Sep 28, 2018

rongjiecomputer commented Oct 1, 2018

laszlocsomor commented Oct 1, 2018

rongjiecomputer commented Oct 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laszlocsomor commented Oct 2, 2018

Choose a reason for hiding this comment

rongjiecomputer Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rongjiecomputer commented Sep 26, 2018 •

edited

Loading

rongjiecomputer Sep 27, 2018 •

edited

Loading

rongjiecomputer Oct 11, 2018 •

edited

Loading