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

CGO merged directory should have usable name. #2930

Merged
merged 4 commits into from
Sep 23, 2021

Conversation

thomas-wk
Copy link
Contributor

cgo2 merges sources and generated code under a common directory, however this directory is anonymous.

This breaks nogo's ability to easily include/exclude files as only the file's basename is retained.

This change embeds cgo/packagePath into the merged directory to make it easy to include/exclude sources being compiled by cgo.

Note: PR #2863 ensured that files //line: directives had their meaningful alt names included in nogo output. This is helpful, however for pure go files that are part of a package that uses cgo, there is nothing that adds //line: directives, and the builder will just copy/link pure files into the into the merged cgo folder.

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?
Makes the merged cgo temp directory include the package path for easy inclusion/exclusion from nogo rules.

Which issues(s) does this PR fix?

Fixes # 2862

Other notes for review

cgo2 merges sources and generated code under a common directory, however this directory is anonymous.

This breaks nogo's ability to easily include/exclude files as only the file's basename is retained.

This change embeds cgo/packagePath into the merged directory to make it easy to include/exclude sources being compiled by cgo.

Note: PR bazel-contrib#2863 ensured that files //line: directives had their meaningful alt names included in nogo output, however cgo2's copy and link steps do not add that directive (pieces of the toolchain do), making certain files still challenging to include/exclude.
@google-cla google-cla bot added the cla: yes label Jul 28, 2021
@robfig
Copy link
Contributor

robfig commented Sep 23, 2021

Thank you for the contribution with unit tests! Merging.

@robfig robfig merged commit 53af1a4 into bazel-contrib:master Sep 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants