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

fix: use tree artifacts via copy_directory with exports_directories_only #2996

Merged
merged 7 commits into from
Jan 11, 2022

Conversation

gregmagolan
Copy link
Collaborator

@gregmagolan gregmagolan commented Oct 4, 2021

Fixes the issue with exports_directories_only on RBE.

Removes has_directories from ExternalNpmPackageInfo since it is expected that source directories
are no longer used. This simplified npm_umd_bundle.bzl impl which was using this to filtering; it can now
just check File.is_directory since source directories are no longer passed in.

@@ -22,7 +22,6 @@ ExternalNpmPackageInfo = provider(
doc = "Provides information about one or more external npm packages",
fields = {
"direct_sources": "Depset of direct source files in these external npm package(s)",
"has_directories": "True if any sources are directories",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is no longer needed since we no longer have this case as exports_directories_only now produced proper individually linkable js_library targets with package names instead of a js_library with a source directory and its package name set to the special case $node_modules_dir$

@gregmagolan gregmagolan force-pushed the copy_directory branch 2 times, most recently from 5c1bb1b to 113cfc6 Compare October 4, 2021 03:35
Copy link
Collaborator

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

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

Had a quick scan over it, looking good. LMK when it's green and I'll do a more careful review

@google-cla
Copy link

google-cla bot commented Oct 5, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla
Copy link

google-cla bot commented Oct 5, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla
Copy link

google-cla bot commented Oct 5, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@alexeagle
Copy link
Collaborator

@googlebot I consent.

@google-cla google-cla bot added cla: yes and removed cla: no labels Oct 5, 2021
@alexeagle alexeagle force-pushed the copy_directory branch 3 times, most recently from be6881d to 34a3a68 Compare October 5, 2021 03:48
@matthewjh
Copy link
Contributor

@gregmagolan what's the motivation behind this change?

@alexeagle
Copy link
Collaborator

It fixes the exports_directories_only feature to work with RBE and with --nobuild_runfile_links

The feature originally depended on Bazel accidentally not noticing that inputs were directories, which happened to work in almost all cases. Now we make them TreeArtifacts which unfortunately bazel can't produce as InputArtifacts so a copy operation is needed.

@matthewjh
Copy link
Contributor

matthewjh commented Oct 7, 2021

Thanks @alexeagle. I am about to set up RBE for my org. From what you say it sounds like I will have to turn off exports_directories_only for things to work, until this lands? Or does it only not work with both exports_directories_only and --nobuild_runfile_links?

@alexeagle
Copy link
Collaborator

Doesn't work with either, is my understanding

@alexeagle alexeagle force-pushed the copy_directory branch 2 times, most recently from 34a3a68 to 5fad95a Compare October 9, 2021 00:44
@gregmagolan gregmagolan force-pushed the copy_directory branch 7 times, most recently from ce7ae09 to 7e8709f Compare January 11, 2022 08:22
@gregmagolan gregmagolan force-pushed the copy_directory branch 11 times, most recently from 03651df to 426d4b4 Compare January 11, 2022 16:15
@alexeagle alexeagle merged commit 0d93719 into bazel-contrib:5.x Jan 11, 2022
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.

3 participants