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

[internal] Hook up check to Go #13322

Merged
merged 1 commit into from
Oct 22, 2021
Merged

Conversation

Eric-Arellano
Copy link
Contributor

@Eric-Arellano Eric-Arellano commented Oct 22, 2021

This lets you compile a package without needing to run tests or package a binary.

Note that you can only directly compile a first-party package - to check if a third-party package is buildable, it must be a dependency of a first-party package. Works around #13175.

This does not yet have an optimal UX. It's overly chatty:

❯ ./pants check testprojects/src/go::
13:36:38.57 [INFO] Initializing scheduler...
13:36:39.20 [INFO] Scheduler initialized.
13:36:39.71 [ERROR] Completed: pants.backend.go.goals.check.check_go - go failed (exit code 1).
Partition #1 - github.com/pantsbuild/pants/testprojects/src/go/pants_test:
./testprojects/src/go/pants_test/foo.go:3:1: syntax error: non-declaration statement outside function body

Partition #2 - github.com/pantsbuild/pants/testprojects/src/go/pants_test/bar:



𐄂 go failed.

We also should be streaming the result of each compilation as it happens, which has an added benefit of that output happening when compilation happens as a side effect of run, package, and test. Those fixes will be in a followup.

[ci skip-rust]
[ci skip-build-wheels]

@Eric-Arellano Eric-Arellano force-pushed the go-check branch 2 times, most recently from 1cd9a53 to 94361c2 Compare October 22, 2021 20:41
@Eric-Arellano Eric-Arellano changed the title [WIP] Hook up check to Go [internal] Hook up check to Go Oct 22, 2021
[ci skip-rust]
[ci skip-build-wheels]
@Eric-Arellano Eric-Arellano requested a review from tdyas October 22, 2021 20:44
@Eric-Arellano Eric-Arellano marked this pull request as ready for review October 22, 2021 20:44
Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

We also should be streaming the result of each compilation as it happens, which has an added benefit of that output happening when compilation happens as a side effect of run, package, and test. Those fixes will be in a followup.

Not a blocker, but if you do end up making changes here, this is pretty easy to do... mostly just adding def message and def level methods to FallibleBuiltGoPackage.

Comment on lines +33 to +36
build_requests = await MultiGet(
Get(FallibleBuildGoPackageRequest, BuildGoPackageTargetRequest(field_set.address))
for field_set in request.field_sets
)
Copy link
Member

Choose a reason for hiding this comment

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

This would probably represent a fatal error, right? i.e., something that would prevent anything from compiling? If so, probably fine for it not to be fallible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #13334. We run go list to determine how to build a package, and also its decencies for inference etc. It's important that syntax errors there don't break the entire build.

Copy link
Member

Choose a reason for hiding this comment

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

In Python, I think that we skip inference in that case, and the effect is that whichever tool you were attempting to use will fail with the parse error instead (I just observed this with mypy reporting a parse error, which after fixing allowed inference to get further). But yea, either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly. That PR has the same effect. We give up on inference if we fail to run go list

@Eric-Arellano Eric-Arellano merged commit 16dd4e0 into pantsbuild:main Oct 22, 2021
@Eric-Arellano Eric-Arellano deleted the go-check branch October 22, 2021 21:36
@wisechengyi wisechengyi mentioned this pull request Oct 24, 2021
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