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

Consider running CI under Windows #2686

Closed
jbduncan opened this issue Dec 15, 2016 · 9 comments
Closed

Consider running CI under Windows #2686

jbduncan opened this issue Dec 15, 2016 · 9 comments
Assignees
Labels
P2 package=general status=triaged type=other Miscellaneous activities not covered by other type= labels

Comments

@jbduncan
Copy link
Contributor

jbduncan commented Dec 15, 2016

As is currently done in google-java-format, I think it's worth testing Guava against Windows using AppVeyor, or at least I think it's worth for the Guava team to consider looking into it.

Currently, Guava makes no guarantees that it will run successfully on Windows, as explained in the README:

IMPORTANT WARNINGS
...
5, We unit-test and benchmark the libraries using only OpenJDK 1.7 on Linux. Some features, especially in com.google.common.io, may not work correctly in other environments.

I do understand that there would need to be a number of changes or improvements before Guava can build successfully on Windows (#2130), so I suspect this would be a long-term thing to tackle.

@raghsriniv raghsriniv added type=other Miscellaneous activities not covered by other type= labels and removed type=dev labels Aug 5, 2019
@cpovirk cpovirk changed the title Consider using AppVeyor Consider running CI under Windows Apr 17, 2021
@cpovirk
Copy link
Member

cpovirk commented May 30, 2023

@cgdecker pointed out that such testing would have caught the problem he identified with the Guava 32.0.0 reimplementation Files.createTempDir and FileBackedOutputStream. In fairness, he does also remind me of the same IMPORTANT WARNING that you quoted above :)

@jbduncan
Copy link
Contributor Author

@cpovirk Oh no! Yeah, the potential incompatibility between the new implementations and Windows occurred to me earlier today. 😆

copybara-service bot pushed a commit that referenced this issue May 31, 2023
…rk under Windows anymore (if ever they did).

Relates to:
- #4011
- #2575
- #2686

Also, fix some Javadoc.

RELNOTES=n/a
PiperOrigin-RevId: 536467940
copybara-service bot pushed a commit that referenced this issue May 31, 2023
…rk under Windows anymore (if ever they did).

Relates to:
- #4011
- #2575
- #2686

Also, fix some Javadoc.

RELNOTES=n/a
PiperOrigin-RevId: 536752422
copybara-service bot pushed a commit that referenced this issue Jun 7, 2023
Based on [some discussions in GitHub](#6535 (comment)), I'm under the impression that Windows would create the temporary directory/file in a secure location even if our call to `java.nio.file.Files.createTempDirectory`/`createTempFile` passes no ACL attribute. However, in case we are in an unusual situation (Linux with NFS temporary directory???) in which ACLs are supported but the temporary directory is _not_ a secure location, I've arranged for the file to be created with an ACL that grants permissions only to the current user. I set the user's permissions to the ones that I saw on a file created in the temporary directory under Java's default settings, and I didn't do anything to set the additional permissions I was seeing for administrators. The resulting file's permissions look plausibly correct in the Windows property dialog, if slightly different than what I get when creating a file/directory myself through Explorer.

Fixes #6535

Relates to:
- #4011
- #2575
- #2686. (It sure would be convenient to have Windows CI set up!)

RELNOTES=`io`: Fixed `Files.createTempDir` and `FileBackedOutputStream` under Windows.
PiperOrigin-RevId: 538492945
copybara-service bot pushed a commit that referenced this issue Jun 7, 2023
Based on [some discussions in GitHub](#6535 (comment)), I'm under the impression that Windows would create the temporary directory/file in a secure location even if our call to `java.nio.file.Files.createTempDirectory`/`createTempFile` passes no ACL attribute. However, in case we are in an unusual situation (Linux with NFS temporary directory???) in which ACLs are supported but the temporary directory is _not_ a secure location, I've arranged for the file to be created with an ACL that grants permissions only to the current user. I set the user's permissions to the ones that I saw on a file created in the temporary directory under Java's default settings, and I didn't do anything to set the additional permissions I was seeing for administrators. The resulting file's permissions look plausibly correct in the Windows property dialog, if slightly different than what I get when creating a file/directory myself through Explorer.

Fixes #6535

Relates to:
- #4011
- #2575
- #2686. (It sure would be convenient to have Windows CI set up!)

RELNOTES=`io`: Fixed `Files.createTempDir` and `FileBackedOutputStream` under Windows.
PiperOrigin-RevId: 538492945
copybara-service bot pushed a commit that referenced this issue Jun 7, 2023
Based on [some discussions in GitHub](#6535 (comment)), I'm under the impression that Windows would create the temporary directory/file in a secure location even if our call to `java.nio.file.Files.createTempDirectory`/`createTempFile` passes no ACL attribute. However, in case we are in an unusual situation (Linux with NFS temporary directory???) in which ACLs are supported but the temporary directory is _not_ a secure location, I've arranged for the file to be created with an ACL that grants permissions only to the current user. I set the user's permissions to the ones that I saw on a file created in the temporary directory under Java's default settings, and I didn't do anything to set the additional permissions I was seeing for administrators. The resulting file's permissions look plausibly correct in the Windows property dialog, if slightly different than what I get when creating a file/directory myself through Explorer.

Fixes #6535

Relates to:
- #4011
- #2575
- #2686. (It sure would be convenient to have Windows CI set up!)

RELNOTES=`io`: Fixed `Files.createTempDir` and `FileBackedOutputStream` under Windows.
PiperOrigin-RevId: 538492945
@cpovirk
Copy link
Member

cpovirk commented Jun 8, 2023

This probably did get a lot easier with the switch of our CI to GitHub Actions. It might be roughly just this...

https://github.com/google/error-prone/blob/3602ab040d4dc0f7348fe6103ccfd20252ec1af9/.github/workflows/ci.yml#L27-L29

...and then some number of if (isWindows()) { return; } or assumeThat(isWindows()).isFalse() checks in our tests... :) But #2130 suggests that even that might not be too bad....

I've been trying to remember if the tests would have caught any production problems under Windows (other than, you know, the significant recent one...). I think we knew about Files.deleteRecursively (#365) before it was reported (and I think a similar problem exists even for Linux?), and I'm not sure if #3402 was Windows-specific, either—though perhaps the default Windows temporary directory would have triggered the bug, and that would have been useful.

cpovirk added a commit to cpovirk/guava that referenced this issue Jun 8, 2023
DO NOT SUBMIT

Fixes google#2686 if we submit it, which
we won't.
@cpovirk
Copy link
Member

cpovirk commented Jun 8, 2023

Now playing with #6542...

Our "print surefire reports" step seems likely to go poorly, given that it runs a shell script, but maybe I'll be surprised?

@cpovirk
Copy link
Member

cpovirk commented Jun 8, 2023

Well, we might need to skip another test, too:

Linux:

Running com.google.common.util.concurrent.AbstractFutureFallbackAtomicHelperTest
Tests run: 43, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 102.883 sec

Windows:

Running com.google.common.util.concurrent.AbstractFutureFallbackAtomicHelperTest
Tests run: 43, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 2,072.23 sec

@cpovirk
Copy link
Member

cpovirk commented Jun 8, 2023

Similarly:

Running com.google.common.util.concurrent.AbstractFutureTest
Tests run: 43, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1,096.901 sec
Running com.google.common.util.concurrent.AbstractFutureTest
Tests run: 43, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 52.41 sec

@cpovirk
Copy link
Member

cpovirk commented Jun 8, 2023

And the failures so far, as it works to finish testing util.concurrent:

Running com.google.common.base.ThrowablesTest
Tests run: 44, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.078 sec <<< FAILURE!
Running com.google.common.io.FileBackedOutputStreamAndroidIncompatibleTest
Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.015 sec <<< FAILURE!
Running com.google.common.io.FileBackedOutputStreamTest
Tests run: 4, Failures: 0, Errors: 3, Skipped: 0, Time elapsed: 0 sec <<< FAILURE!
Running com.google.common.io.FilesCreateTempDirTest
Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0 sec <<< FAILURE!
Running com.google.common.io.MoreFilesTest
Tests run: 1151, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 2.862 sec <<< FAILURE!
Running com.google.common.io.ResourcesTest
Tests run: 773, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 1.016 sec <<< FAILURE!
Running com.google.common.reflect.ClassPathTest
Tests run: 44, Failures: 7, Errors: 1, Skipped: 0, Time elapsed: 6.936 sec <<< FAILURE!

@cpovirk cpovirk added P2 and removed P3 labels Jun 8, 2023
@cpovirk cpovirk self-assigned this Jun 8, 2023
@jbduncan
Copy link
Contributor Author

jbduncan commented Jun 8, 2023

Interesting. 🤔

Any idea why these tests fail or take so long?

@cpovirk
Copy link
Member

cpovirk commented Jun 8, 2023

I think I generally know what's up. And fortunately, the Surefire logs did get printed after all, so I'm in the process of confirming.

copybara-service bot pushed a commit that referenced this issue Jun 8, 2023
- Fixes #2686
- In some sense addresses #2130, but I'm going to leave that open to track removing the suppressions.
- Provides better testing for the fix for #6535

RELNOTES=n/a
PiperOrigin-RevId: 538841996
copybara-service bot pushed a commit that referenced this issue Jun 8, 2023
- Fixes #2686
- In some sense addresses #2130, but I'm going to leave that open to track removing the suppressions.
- Provides better testing for the fix for #6535

RELNOTES=n/a
PiperOrigin-RevId: 538841996
copybara-service bot pushed a commit that referenced this issue Jun 8, 2023
- Fixes #2686
- In some sense addresses #2130, but I'm going to leave that open to track removing the suppressions.
- Provides better testing for the fix for #6535

RELNOTES=n/a
PiperOrigin-RevId: 538841996
copybara-service bot pushed a commit that referenced this issue Jun 8, 2023
Based on [some discussions in GitHub](#6535 (comment)), I'm under the impression that Windows would create the temporary directory/file in a secure location even if our call to `java.nio.file.Files.createTempDirectory`/`createTempFile` passes no ACL attribute. However, in case we are in an unusual situation (Linux with NFS temporary directory???) in which ACLs are supported but the temporary directory is _not_ a secure location, I've arranged for the file to be created with an ACL that grants permissions only to the current user. I set the user's permissions to the ones that I saw on a file created in the temporary directory under Java's default settings, and I didn't do anything to set the additional permissions I was seeing for administrators. The resulting file's permissions look plausibly correct in the Windows property dialog, if slightly different than what I get when creating a file/directory myself through Explorer.

Fixes #6535

Relates to:
- #4011
- #2575
- #2686 (Yay, we have Windows CI set up!)
- #2130

RELNOTES=`io`: Fixed `Files.createTempDir` and `FileBackedOutputStream` under Windows.
PiperOrigin-RevId: 538492945
copybara-service bot pushed a commit that referenced this issue Jun 8, 2023
Based on [some discussions in GitHub](#6535 (comment)), I'm under the impression that Windows would create the temporary directory/file in a secure location even if our call to `java.nio.file.Files.createTempDirectory`/`createTempFile` passes no ACL attribute. However, in case we are in an unusual situation (Linux with NFS temporary directory???) in which ACLs are supported but the temporary directory is _not_ a secure location, I've arranged for the file to be created with an ACL that grants permissions only to the current user. I set the user's permissions to the ones that I saw on a file created in the temporary directory under Java's default settings, and I didn't do anything to set the additional permissions I was seeing for administrators. The resulting file's permissions look plausibly correct in the Windows property dialog, if slightly different than what I get when creating a file/directory myself through Explorer.

Fixes #6535

Relates to:
- #4011
- #2575
- #2686 (Yay, we have Windows CI set up!)
- #2130

RELNOTES=`io`: Fixed `Files.createTempDir` and `FileBackedOutputStream` under Windows.
PiperOrigin-RevId: 538492945
copybara-service bot pushed a commit that referenced this issue Jun 8, 2023
Based on [some discussions in GitHub](#6535 (comment)), I'm under the impression that Windows would create the temporary directory/file in a secure location even if our call to `java.nio.file.Files.createTempDirectory`/`createTempFile` passes no ACL attribute. However, in case we are in an unusual situation (Linux with NFS temporary directory???) in which ACLs are supported but the temporary directory is _not_ a secure location, I've arranged for the file to be created with an ACL that grants permissions only to the current user. I set the user's permissions to the ones that I saw on a file created in the temporary directory under Java's default settings, and I didn't do anything to set the additional permissions I was seeing for administrators. The resulting file's permissions look plausibly correct in the Windows property dialog, if slightly different than what I get when creating a file/directory myself through Explorer.

Fixes #6535

Relates to:
- #4011
- #2575
- #2686 (Yay, we have Windows CI set up!)
- #2130

RELNOTES=`io`: Fixed `Files.createTempDir` and `FileBackedOutputStream` under Windows.
PiperOrigin-RevId: 538492945
copybara-service bot pushed a commit that referenced this issue Jun 8, 2023
Based on [some discussions in GitHub](#6535 (comment)), I'm under the impression that Windows would create the temporary directory/file in a secure location even if our call to `java.nio.file.Files.createTempDirectory`/`createTempFile` passes no ACL attribute. However, in case we are in an unusual situation (Linux with NFS temporary directory???) in which ACLs are supported but the temporary directory is _not_ a secure location, I've arranged for the file to be created with an ACL that grants permissions only to the current user. I set the user's permissions to the ones that I saw on a file created in the temporary directory under Java's default settings, and I didn't do anything to set the additional permissions I was seeing for administrators. The resulting file's permissions look plausibly correct in the Windows property dialog, if slightly different than what I get when creating a file/directory myself through Explorer.

Fixes #6535

Relates to:
- #4011
- #2575
- #2686 (Yay, we have Windows CI set up!)
- #2130

RELNOTES=`io`: Fixed `Files.createTempDir` and `FileBackedOutputStream` under Windows.
PiperOrigin-RevId: 538492945
copybara-service bot pushed a commit that referenced this issue Jun 8, 2023
Based on [some discussions in GitHub](#6535 (comment)), I'm under the impression that Windows would create the temporary directory/file in a secure location even if our call to `java.nio.file.Files.createTempDirectory`/`createTempFile` passes no ACL attribute. However, in case we are in an unusual situation (Linux with NFS temporary directory???) in which ACLs are supported but the temporary directory is _not_ a secure location, I've arranged for the file to be created with an ACL that grants permissions only to the current user. I set the user's permissions to the ones that I saw on a file created in the temporary directory under Java's default settings, and I didn't do anything to set the additional permissions I was seeing for administrators. The resulting file's permissions look plausibly correct in the Windows property dialog, if slightly different than what I get when creating a file/directory myself through Explorer.

Fixes #6535

Relates to:
- #4011
- #2575
- #2686 (Yay, we have Windows CI set up!)
- #2130

RELNOTES=`io`: Fixed `Files.createTempDir` and `FileBackedOutputStream` under Windows.
PiperOrigin-RevId: 538888769
copybara-service bot pushed a commit that referenced this issue Jun 15, 2023
Also, update documentation to reflect that we test under:
- [Windows](#2686)
- [Java 17](#5801)

RELNOTES=n/a
PiperOrigin-RevId: 539045930
copybara-service bot pushed a commit that referenced this issue Jun 15, 2023
Also, update documentation to reflect that we test under:
- [Windows](#2686)
- [Java 17](#5801)

RELNOTES=n/a
PiperOrigin-RevId: 540650887
cpovirk added a commit that referenced this issue Jun 16, 2023
copybara-service bot pushed a commit that referenced this issue Sep 26, 2023
…_services_, a rare use case.

Fixes #6634

Relevant to #2686 in that it shows that we would ideally run our Windows testing under both Java 8 *and* a newer version....

RELNOTES=`io`: Fixed `Files.createTempDir` and `FileBackedOutputStream` under [Windows _services_, a rare use case](#6634).
PiperOrigin-RevId: 568604081
cpovirk added a commit that referenced this issue Sep 26, 2023
Fix `Files.createTempDir` and `FileBackedOutputStream` under Windows _services_, a rare use case.

Fixes #6634

Relevant to #2686 in that it shows that we would ideally run our Windows testing under both Java 8 *and* a newer version....

RELNOTES=`io`: Fixed `Files.createTempDir` and `FileBackedOutputStream` under [Windows _services_, a rare use case](#6634).
PiperOrigin-RevId: 568604081
cpovirk added a commit that referenced this issue Sep 26, 2023
…ting

Fix `Files.createTempDir` and `FileBackedOutputStream` under Windows _services_, a rare use case.

Fixes #6634

Relevant to #2686 in that it shows that we would ideally run our Windows testing under both Java 8 *and* a newer version....

RELNOTES=`io`: Fixed `Files.createTempDir` and `FileBackedOutputStream` under [Windows _services_, a rare use case](#6634).
PiperOrigin-RevId: 568604081
copybara-service bot pushed a commit that referenced this issue Sep 26, 2023
…_services_, a rare use case.

Fixes #6634

Relevant to #2686 in that it shows that we would ideally run our Windows testing under both Java 8 *and* a newer version....

RELNOTES=`io`: Fixed `Files.createTempDir` and `FileBackedOutputStream` under [Windows _services_, a rare use case](#6634).
PiperOrigin-RevId: 568604081
copybara-service bot pushed a commit that referenced this issue Sep 26, 2023
…_services_, a rare use case.

Fixes #6634

Relevant to #2686 in that it shows that we would ideally run our Windows testing under both Java 8 *and* a newer version....

RELNOTES=`io`: Fixed `Files.createTempDir` and `FileBackedOutputStream` under [Windows _services_, a rare use case](#6634).
PiperOrigin-RevId: 568604081
copybara-service bot pushed a commit that referenced this issue Oct 2, 2023
I've found them to be [flaky](#6731 (comment)). We already [skip](#2130) some tests under Windows, so what's two more (especially only under Java 8, which I didn't include when setting up [Windows CI](#2686) and which I was testing only as part of #6634)?

RELNOTES=n/a
PiperOrigin-RevId: 568645480
copybara-service bot pushed a commit that referenced this issue Oct 2, 2023
I've found them to be [flaky](#6731 (comment)). We already [skip](#2130) some tests under Windows, so what's two more (especially only under Java 8, which I didn't include when setting up [Windows CI](#2686) and which I was testing only as part of #6634)?

RELNOTES=n/a
PiperOrigin-RevId: 570103461
copybara-service bot pushed a commit that referenced this issue Oct 6, 2023
…_services_ (at least under JDK 9+), a rare use case.

Fixes #6634

Relevant to #2686 in that it shows that we would ideally run our Windows testing under both Java 8 *and* a newer version....

RELNOTES=`io`: Fixed `Files.createTempDir` and `FileBackedOutputStream` under [Windows _services_, a rare use case](#6634). (The fix actually covers only Java 9+ because Java 8 would require an additional approach. Let us know if you need support under Java 8.)
PiperOrigin-RevId: 568604081
copybara-service bot pushed a commit that referenced this issue Oct 6, 2023
…_services_ (at least under JDK 9+), a rare use case.

Fixes #6634

Relevant to #2686 in that it shows that we would ideally run our Windows testing under both Java 8 *and* a newer version....

RELNOTES=`io`: Fixed `Files.createTempDir` and `FileBackedOutputStream` under [Windows _services_, a rare use case](#6634). (The fix actually covers only Java 9+ because Java 8 would require an additional approach. Let us know if you need support under Java 8.)
PiperOrigin-RevId: 571368120
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 package=general status=triaged type=other Miscellaneous activities not covered by other type= labels
Projects
None yet
4 participants