search

eisop / checker-framework

Pluggable type-checking for Java
https://eisop.github.io/
Other
17 stars 16 forks source link

Fix path handling to work on Windows #691

Closed WeilanTao closed 3 months ago

WeilanTao commented 6 months ago

This pr enables CF run on Windows, which passes the simple nullness check.

Ao-senXiong commented 6 months ago

@WeilanTao Can you describe the purpose of this PR in description section?

wmdietl commented 6 months ago

Could you add Windows as a target to the azure pipeline and run one of the JUnit tests? We cannot run all tests, b/c they depend on Makefiles, etc. But the simple JUnit tests should work after this change, right? Also, don't do all JDK versions, just whatever the newest LTS JDK is on Windows.

wmdietl commented 6 months ago

Could you add Windows as a target to the azure pipeline and run one of the JUnit tests? We cannot run all tests, b/c they depend on Makefiles, etc. But the simple JUnit tests should work after this change, right? Also, don't do all JDK versions, just whatever the newest LTS JDK is on Windows.

Can you make the PR that add a Windows target separately? Then we'll see that running on Windows fails for that PR. Then we merge that other PR into this PR and it should pass.

WeilanTao commented 6 months ago

Could you add Windows as a target to the azure pipeline and run one of the JUnit tests? We cannot run all tests, b/c they depend on Makefiles, etc. But the simple JUnit tests should work after this change, right? Also, don't do all JDK versions, just whatever the newest LTS JDK is on Windows.

How can I add Windows as a target to azure?

wmdietl commented 6 months ago

Could you add Windows as a target to the azure pipeline and run one of the JUnit tests? We cannot run all tests, b/c they depend on Makefiles, etc. But the simple JUnit tests should work after this change, right? Also, don't do all JDK versions, just whatever the newest LTS JDK is on Windows.

How can I add Windows as a target to azure?

See here: https://learn.microsoft.com/en-us/azure/devops/pipelines/agents/hosted?view=azure-devops&tabs=yaml#software

For example, we use it here: https://github.com/eisopux/checker-framework-vscode/blob/97d3d0d3fcdc4c749d739d98c8def69ba9beea0c/azure-pipelines.yml#L11

wmdietl commented 6 months ago

I've started https://github.com/eisop/checker-framework/pull/698 It doesn't look like finding the JUnit tests is the problem.

wmdietl commented 6 months ago

I've started #698 It doesn't look like finding the JUnit tests is the problem.

It actually is, at least one of the problems: https://dev.azure.com/eisop/checker-framework/_build/results?buildId=4527&view=logs&j=bfccef3f-45d4-504d-9a7f-a8a74922e6bf&t=b7898372-a000-5f18-bb5b-13b9fc43dfa8&l=37

Task 'NullnessTest' not found in root project 'checker-framework' and its subprojects.

Your change here might fix this.

WeilanTao commented 6 months ago

I've started #698 It doesn't look like finding the JUnit tests is the problem.

It actually is, at least one of the problems: https://dev.azure.com/eisop/checker-framework/_build/results?buildId=4527&view=logs&j=bfccef3f-45d4-504d-9a7f-a8a74922e6bf&t=b7898372-a000-5f18-bb5b-13b9fc43dfa8&l=37

Task 'NullnessTest' not found in root project 'checker-framework' and its subprojects.

Your change here might fix this.

Thank you for bringing it up, professor. I am still in the step of creating the Windows-targeted job on azure-pipline.yml. It took me some time to learn it. But now I've seen it on #698. Thank you for that. I will check the regex issue that you mentioned.

Ao-senXiong commented 6 months ago

I tried in your branch on my windows machine, ./gradlew assembleforjava works but ./gradlew test is still failing.

wmdietl commented 6 months ago

I tried in your branch on my windows machine, ./gradlew assembleforjava works but ./gradlew test is still failing.

@Ao-senXiong What failures do you get? In #698 just running test passed.

wmdietl commented 6 months ago

@WeilanTao Can you pull in the changes from #698 into this branch so we'll see whether things pass now?

One thing to notice: #698 still runs bash, so we don't really test using the gradlew.bat file. Can you look whether there is a way to use normal cmd in Azure Pipeline?

WeilanTao commented 6 months ago

@WeilanTao Can you pull in the changes from #698 into this branch so we'll see whether things pass now?

One thing to notice: #698 still runs bash, so we don't really test using the gradlew.bat file. Can you look whether there is a way to use normal cmd in Azure Pipeline?

I will try to pull it first and see.

wmdietl commented 6 months ago

@WeilanTao It looks like your path matching fix is working correctly and the RegexTest target is now executed. Great! However, we're back to the test mismatch that you had observed before. If you look at the CI output, you'll see:

org.checkerframework.checker.test.junit.RegexTest > run[D:\a\1\s\checker\tests\regex] FAILED
    java.lang.StringIndexOutOfBoundsException: String index out of range: -66

org.checkerframework.checker.test.junit.RegexTest > run[D:\a\1\s\checker\tests\regex_poly] STANDARD_OUT
    To see the javac command line and output, run with: -Pemit.test.debug

org.checkerframework.checker.test.junit.RegexTest > run[D:\a\1\s\checker\tests\regex_poly] FAILED
    java.lang.AssertionError: 0 out of 11 expected diagnostics were found.
    11 unexpected diagnostics were found:
      PolyRegexTests.java:-1: other: :18: error: (assignment.type.incompatible)
            @Regex String s = method(str); // error
....
    11 expected diagnostics were not found:
      PolyRegexTests.java:18: error: (assignment.type.incompatible)

In regex there is an unexpected StringIndexOutOfBoundsException. Please try to minimize for which test case in that directory the exception happens.

In regex_poly it looks like all the right errors are produced, but something about the error format doesn't work as expected. Notice the -1: other: part. Please have a look at where this matching happens and where the unexpected part comes from.

Ao-senXiong commented 6 months ago

I tried in your branch on my windows machine, ./gradlew assembleforjava works but ./gradlew test is still failing.

@Ao-senXiong What failures do you get? In #698 just running test passed.

It says that 

PS C:\Users\aosen\Documents\eisop-windows\checker-framework> .\gradlew test
Starting a Gradle Daemon (subsequent builds will be faster)                                                             
> Task :cloneAndBuildJSpecify FAILED
Building JSpecify repository.
Executing in directory: C:\Users\aosen\Documents\eisop-windows/jspecify

> Task :checker-qual:jar
warning: Classpath is empty. Private-Package, -privatepackage, and Export-Package can only expand from the classpath when there is one

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':cloneAndBuildJSpecify'.
 A problem occurred starting process 'command 'sh''
wmdietl commented 6 months ago

@Ao-senXiong The cloneAndBuildJSpecify target was removed in #693, so please pull in master into whatever branch you're testing.

Azure on Windows seems to still use a bash shell. Can you look into how to use cmd instead? See https://github.com/eisop/checker-framework/pull/691#issuecomment-1925776641

WeilanTao commented 6 months ago

@WeilanTao It looks like your path matching fix is working correctly and the RegexTest target is now executed. Great! However, we're back to the test mismatch that you had observed before. If you look at the CI output, you'll see:

org.checkerframework.checker.test.junit.RegexTest > run[D:\a\1\s\checker\tests\regex] FAILED
    java.lang.StringIndexOutOfBoundsException: String index out of range: -66

org.checkerframework.checker.test.junit.RegexTest > run[D:\a\1\s\checker\tests\regex_poly] STANDARD_OUT
    To see the javac command line and output, run with: -Pemit.test.debug

org.checkerframework.checker.test.junit.RegexTest > run[D:\a\1\s\checker\tests\regex_poly] FAILED
    java.lang.AssertionError: 0 out of 11 expected diagnostics were found.
    11 unexpected diagnostics were found:
      PolyRegexTests.java:-1: other: :18: error: (assignment.type.incompatible)
            @Regex String s = method(str); // error
....
    11 expected diagnostics were not found:
      PolyRegexTests.java:18: error: (assignment.type.incompatible)

In regex there is an unexpected StringIndexOutOfBoundsException. Please try to minimize for which test case in that directory the exception happens.

In regex_poly it looks like all the right errors are produced, but something about the error format doesn't work as expected. Notice the -1: other: part. Please have a look at where this matching happens and where the unexpected part comes from.

wmdietl commented 6 months ago

@WeilanTao Please don't use edits of comments for substantive changes. I didn't get a notification about all the additions you made.

You should be able to use https://github.com/eisop/checker-framework/blob/master/checker/bin/javac.bat (or just javac if you have bash) on your Windows console. And then javac -processor regex checker/tests/regex/*.java. That should show you all the error messages for those test files.

The regex annotations are here: https://github.com/eisop/checker-framework/tree/master/checker-qual/src/main/java/org/checkerframework/checker/regex/qual

I think the -1: other comes from these files: https://github.com/eisop/checker-framework/blob/276cdd2c036f6d82553e2eaf4a9824f0ba33f458/framework-test/src/main/java/org/checkerframework/framework/test/diagnostics/DiagnosticKind.java#L15

In particular, maybe one of the regexes here needs adjusting for Windows: https://github.com/eisop/checker-framework/blob/276cdd2c036f6d82553e2eaf4a9824f0ba33f458/framework-test/src/main/java/org/checkerframework/framework/test/diagnostics/TestDiagnosticUtils.java#L24

WeilanTao commented 6 months ago

@WeilanTao Please don't use edits of comments for substantive changes. I didn't get a notification about all the additions you made.

You should be able to use https://github.com/eisop/checker-framework/blob/master/checker/bin/javac.bat (or just javac if you have bash) on your Windows console. And then javac -processor regex checker/tests/regex/*.java. That should show you all the error messages for those test files.

The regex annotations are here: https://github.com/eisop/checker-framework/tree/master/checker-qual/src/main/java/org/checkerframework/checker/regex/qual

I think the -1: other comes from these files:

https://github.com/eisop/checker-framework/blob/276cdd2c036f6d82553e2eaf4a9824f0ba33f458/framework-test/src/main/java/org/checkerframework/framework/test/diagnostics/DiagnosticKind.java#L15

In particular, maybe one of the regexes here needs adjusting for Windows:

https://github.com/eisop/checker-framework/blob/276cdd2c036f6d82553e2eaf4a9824f0ba33f458/framework-test/src/main/java/org/checkerframework/framework/test/diagnostics/TestDiagnosticUtils.java#L24

Thank you. Let me look at it

WeilanTao commented 6 months ago

The mismatch of -1: other: in regex-poly seems to be fixed now. The exception of StringIndexOutOfBoundsException in regex doesn't seem to happen on my local

wmdietl commented 5 months ago

The misc CI failure is from:

framework-test/src/main/java/org/checkerframework/framework/test/diagnostics/TestDiagnosticUtils.java:32:32: missing documentation for DIAGNOSTIC_WARNING_IN_JAVA_REGEX
framework-test/src/main/java/org/checkerframework/framework/test/diagnostics/TestDiagnosticUtils.java:34:33: missing documentation for DIAGNOSTIC_WARNING_IN_JAVA_PATTERN

So please add some javadoc to these fields.

wmdietl commented 4 months ago

Thanks for the further cleanups @WeilanTao ! Don't worry about the plume_lib_jdk21 downstream failure. That is happening because of the upgrade to gradle 8.7 in that downstream project and you see similar errors in other recent PRs. I'm looking into a workaround.

WeilanTao commented 4 months ago

Thanks for the further cleanups @WeilanTao ! Don't worry about the plume_lib_jdk21 downstream failure. That is happening because of the upgrade to gradle 8.7 in that downstream project and you see similar errors in other recent PRs. I'm looking into a workaround.

Thank you so much! Can I now work on the byte code thing?

wmdietl commented 4 months ago

Thank you so much! Can I now work on the byte code thing?

Sent you more info by mail. Once CI is working properly again I'll revisit this PR.