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

Add build guards for all Windows imports #191

Merged
merged 5 commits into from
Feb 11, 2021
Merged

Add build guards for all Windows imports #191

merged 5 commits into from
Feb 11, 2021

Conversation

uhthomas
Copy link
Contributor

Fixes #190

@ghost
Copy link

ghost commented Jan 21, 2021

CLA assistant check
All CLA requirements met.

@uhthomas
Copy link
Contributor Author

Note to maintainers: Should build guards be added to all files, regardless of imports? This repository is Windows specific, so it might make sense?

@thaJeztah
Copy link
Contributor

Thanks! I just opened a similar PR before noticing there was one already (closed it now: #193)

Do you think it's worth to consider adding a doc.go in each of these packages, so that they're not empty on "non-windows"? I'm not 100% sure how go mod (and go mod vendor) handles these when vendoring code and the package is "empty" on the platform on which you're executing the go mod vendor.

@thaJeztah
Copy link
Contributor

@kevpar @dcantah ptal

@katiewasnothere
Copy link
Contributor

Do you think it's worth to consider adding a doc.go in each of these packages, so that they're not empty on "non-windows"? I'm not 100% sure how go mod (and go mod vendor) handles these when vendoring code and the package is "empty" on the platform on which you're executing the go mod vendor.

I'm not super sure how they're handled either. If the absence of doc.go causes import errors though I think it's fine to include them to avoid that.

Note to maintainers: Should build guards be added to all files, regardless of imports? This repository is Windows specific, so it might make sense?

I think that's fine as well, especially since it makes any user errors more understandable.

@uhthomas uhthomas requested a review from a team as a code owner February 10, 2021 23:05
@uhthomas
Copy link
Contributor Author

@katiewasnothere re: build guards for all files; I initially like the idea, but it seems odd to constrain files which have no platform-specific requirements. I'll leave it as is for now (protect windows imports).

pkg/etw/wrapper_32.go Outdated Show resolved Hide resolved
Copy link
Member

@kevpar kevpar left a comment

Choose a reason for hiding this comment

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

LGTM

@uhthomas
Copy link
Contributor Author

Hmm, looks like I might actually be missing a few build guards. Give me a moment.

@uhthomas
Copy link
Contributor Author

uhthomas commented Feb 10, 2021

Okay, had to go through and add a bunch of build tags to files which implicitly use Windows-specific code (i.e tests).

Go test seems happy

➜  go-winio git:(master) go test ./...
?       github.com/Microsoft/go-winio   [no test files]
?       github.com/Microsoft/go-winio/backuptar [no test files]
?       github.com/Microsoft/go-winio/pkg/etw   [no test files]
?       github.com/Microsoft/go-winio/wim/lzx   [no test files]

But Bazel does not

➜  wtest git:(master) ✗ bazel test @local_go_winio//...
Starting local Bazel server and connecting to it...
INFO: Analyzed 23 targets (88 packages loaded, 7439 targets configured).
INFO: Found 17 targets and 6 test targets...
ERROR: /home/thomas/.cache/bazel/_bazel_thomas/48d13cdd56897dee270c57999076ee09/external/local_go_winio/tools/etw-provider-gen/BUILD.bazel:16:10: GoLink external/local_go_winio/tools/etw-provider-gen/etw-provider-gen_/etw-provider-gen failed: (Exit 1): builder failed: error executing command bazel-out/host/bin/external/go_sdk/builder link -sdk external/go_sdk -installsuffix linux_amd64 -package_list bazel-out/host/bin/external/go_sdk/packages.txt -o ... (remaining 11 argument(s) skipped)

Use --sandbox_debug to see verbose messages from the sandbox builder failed: error executing command bazel-out/host/bin/external/go_sdk/builder link -sdk external/go_sdk -installsuffix linux_amd64 -package_list bazel-out/host/bin/external/go_sdk/packages.txt -o ... (remaining 11 argument(s) skipped)

Use --sandbox_debug to see verbose messages from the sandbox
external/go_sdk/pkg/tool/linux_amd64/link: bazel-out/k8-fastbuild/bin/external/local_go_winio/tools/etw-provider-gen/etw-provider-gen.a: not package main
link: error running subcommand external/go_sdk/pkg/tool/linux_amd64/link: exit status 2
INFO: Elapsed time: 9.307s, Critical Path: 1.48s
INFO: 96 processes: 26 internal, 70 linux-sandbox.
FAILED: Build did NOT complete successfully
@local_go_winio//:go-winio_test                                       NO STATUS
@local_go_winio//backuptar:backuptar_test                             NO STATUS
@local_go_winio//pkg/etw:etw_test                                     NO STATUS
@local_go_winio//pkg/etwlogrus:etwlogrus_test                         NO STATUS
@local_go_winio//pkg/guid:guid_test                                   NO STATUS
@local_go_winio//pkg/security:security_test                           NO STATUS

FAILED: Build did NOT complete successfully

Looks like it's complaining about there being main packages where there shouldn't. I guess this is up to the reviewer's discretion as to what to do here?

@uhthomas
Copy link
Contributor Author

uhthomas commented Feb 11, 2021

Bazel is now happy following the addition of noop mains for non Windows builds.

➜  wtest git:(master) ✗ bazel test @local_go_winio//...
INFO: Analyzed 23 targets (0 packages loaded, 0 targets configured).
INFO: Found 17 targets and 6 test targets...
INFO: Elapsed time: 0.475s, Critical Path: 0.34s
INFO: 24 processes: 1 internal, 23 linux-sandbox.
INFO: Build completed successfully, 24 total actions
@local_go_winio//:go-winio_test                                          PASSED in 0.0s
@local_go_winio//backuptar:backuptar_test                                PASSED in 0.0s
@local_go_winio//pkg/etw:etw_test                                        PASSED in 0.0s
@local_go_winio//pkg/etwlogrus:etwlogrus_test                            PASSED in 0.0s
@local_go_winio//pkg/guid:guid_test                                      PASSED in 0.0s
@local_go_winio//pkg/security:security_test                              PASSED in 0.0s

Executed 6 out of 6 tests: 6 tests pass.
There were tests whose specified size is too big. Use the --test_verbose_timeout_warnings command line option to see which ones INFO: Build completed successfully, 24 total actions

and just to double check, so is Go test.

➜  go-winio git:(master) go test ./...
?       github.com/Microsoft/go-winio   [no test files]
?       github.com/Microsoft/go-winio/backuptar [no test files]
?       github.com/Microsoft/go-winio/pkg/etw   [no test files]
?       github.com/Microsoft/go-winio/pkg/etw/sample    [no test files]
?       github.com/Microsoft/go-winio/tools/etw-provider-gen    [no test files]
?       github.com/Microsoft/go-winio/wim/lzx   [no test files]
?       github.com/Microsoft/go-winio/wim/validate      [no test files]

Do these changes look okay to you?

Copy link
Contributor

@katiewasnothere katiewasnothere left a comment

Choose a reason for hiding this comment

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

LGTM

@katiewasnothere katiewasnothere merged commit 68cdd9b into microsoft:master Feb 11, 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.

OS-specific imports are not correctly guarded by build tags
4 participants