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

Use EISOP CF sources #118

Merged
merged 35 commits into from
Jan 4, 2024
Merged

Use EISOP CF sources #118

merged 35 commits into from
Jan 4, 2024

Conversation

wmdietl
Copy link
Collaborator

@wmdietl wmdietl commented Dec 8, 2023

This is a follow-up to #116.
Instead of merging into the main branch and breaking CI, this PR merges into a new main-eisop branch. CI for that branch will still fail, but we'll have a clean main branch and can have smaller PRs against main-eisop until that branch passes CI or is considered good enough.

@wmdietl wmdietl requested review from cpovirk and netdpb December 8, 2023 15:44
@@ -69,4 +73,4 @@ git_clone jspecify --no-single-branch

git_clone jdk --depth 1 --single-branch

git_clone checker-framework
git_clone checker-framework --depth 1 --single-branch
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have a SHALLOW environment variable that users can set to enable this. That might be better than hard-coding (which, admittedly, we do for the JDK, but it's truly huge). I don't remember exactly how much thought we gave to all this, though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The point of $SHALLOW is to allow CIs like GitHub actions to avoid cloning any commits they don't care about. If you're just running tests for CI you don't need to check any past commits out.

I think if someone has their own fork of eisop we wouldn't want them to clone only one branch. I would suggest continuing to rely on $SHALLOW (defined externally) for checker-framework.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not really related to this PR, but:

Use --filter=blob:none instead of --depth 1

I wonder if that's worth investigating someday.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had a quick look at this post:
https://github.blog/2020-12-21-get-up-to-speed-with-partial-clone-and-shallow-clone/

It seems to generally prefer the blob-less mode. For CI, where one only has a single build, continuing to use --depth 1 seems okay. But as this script is for developer set-up, using blob-less might be better.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's postpone that to a different PR. It's not related to switching to EISOP.

.github/workflows/build.yml Outdated Show resolved Hide resolved
initialize-project Outdated Show resolved Hide resolved
@@ -69,4 +73,4 @@ git_clone jspecify --no-single-branch

git_clone jdk --depth 1 --single-branch

git_clone checker-framework
git_clone checker-framework --depth 1 --single-branch
Copy link
Collaborator

Choose a reason for hiding this comment

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

The point of $SHALLOW is to allow CIs like GitHub actions to avoid cloning any commits they don't care about. If you're just running tests for CI you don't need to check any past commits out.

I think if someone has their own fork of eisop we wouldn't want them to clone only one branch. I would suggest continuing to rely on $SHALLOW (defined externally) for checker-framework.

@netdpb
Copy link
Collaborator

netdpb commented Dec 12, 2023

Interesting!

tests.ConformanceTest > conformanceTests FAILED
    expected:
        # 10 pass; 6 fail; 16 total; 62.5% score
        ...
    but was:
        # 4 pass; 12 fail; 16 total; 25.0% score

but

tests.ConformanceTest > conformanceTestsOnSamples FAILED
    expected:
        # 75 pass; 498 fail; 573 total; 13.1% score
        ...
    but was:
        # 224 pass; 349 fail; 573 total; 39.1% score

Score went up from 13.1% to 39.1%. Woohoo!

@netdpb
Copy link
Collaborator

netdpb commented Dec 15, 2023

I see a failure of depending on that zip artifact. Do you need to merge main in again?

Once that's fixed, you can run the following to generate changes to the conformance test reports to reflect the current behavior:

JSPECIFY_CONFORMANCE_TEST_MODE=write ./gradelew conformanceTests

@wmdietl
Copy link
Collaborator Author

wmdietl commented Dec 18, 2023

I see a failure of depending on that zip artifact. Do you need to merge main in again?

It looks like this PR is even with main and I don't see a failure about a zip file. Can you point me to the failure?
I think I got this error locally when I hadn't updated my local jspecify clone.

I do see this failure:

> Task :conformanceTests

tests.ConformanceTest > conformanceTests FAILED
    java.nio.file.NoSuchFileException: /home/wdietl/Sync/wmdietl/workspaces/jspecify/jspecify-reference-checker-use-eisop/build/conformanceTests/assertions/org/jspecify/conformance/tests

locally and on CI.

I tried cleaning, updating, and rebuilding everything, but I still get this error.
Did you mean this failure?

@netdpb
Copy link
Collaborator

netdpb commented Dec 18, 2023

Yes, that's the error I mean. That directory should have been created when unzipping.

@wmdietl
Copy link
Collaborator Author

wmdietl commented Dec 19, 2023

Yes, that's the error I mean. That directory should have been created when unzipping.

This was jspecify/jspecify#436 fixed by jspecify/jspecify#437.

@wmdietl wmdietl merged commit da21e35 into jspecify:main-eisop Jan 4, 2024
1 of 2 checks passed
@wmdietl wmdietl deleted the use-eisop branch January 4, 2024 16:00
wmdietl added a commit that referenced this pull request Jan 4, 2024
Co-authored-by: David P. Baker <[email protected]>
wmdietl added a commit that referenced this pull request Jan 9, 2024
Co-authored-by: David P. Baker <[email protected]>
wmdietl added a commit that referenced this pull request Jan 30, 2024
Co-authored-by: David P. Baker <[email protected]>
wmdietl added a commit that referenced this pull request Feb 6, 2024
Co-authored-by: David P. Baker <[email protected]>
wmdietl added a commit that referenced this pull request Apr 10, 2024
Co-authored-by: David P. Baker <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants