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.

Sign up for GitHub

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

Jump to bottom

build(bazel): add data attr to ng_package #22919

Closed
wants to merge 1 commit into from
Closed

build(bazel): add data attr to ng_package #22919

wants to merge 1 commit into from

Conversation

jelbourn
Copy link
Member

@alexeagle One weird thing now is that you can put static files (css, markdown, scss, etc.) into either srcs or data and they will work the same way. I'm not a fan of having two valid ways to do this, which goes back to the question of whether srcs should only accept ts_library and ng_module.

I wasn't sure how to add a test for the error case when srcs includes the output of another rule. Also, it's just checking binDir now, but should we also be doing stuff for bazel-genfiles?

@jelbourn jelbourn requested a review from alexeagle March 21, 2018 23:31
@jelbourn jelbourn added the hotlist: components team Related to Angular CDK or Angular Material label Mar 21, 2018
@mary-poppins
Copy link

You can preview 043fd58 at https://pr22919-043fd58.ngbuilds.io/.

alexeagle
alexeagle reviewed Mar 22, 2018
View reviewed changes
packages/bazel/test/ng_package/example/BUILD.bazel Outdated
name = "arbitrary_text_file",
outs = ["arbitrary_text.txt"],
cmd = "echo Hello > $@",
output_to_bindir = True,
Copy link
Contributor

Choose a reason for hiding this comment

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

also add one with output_to_bindir = False to test the bazel-genfiles prefix gets stripped?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

packages/bazel/src/ng_package/ng_package.bzl Outdated
@@ -212,6 +213,7 @@ def _ng_package_impl(ctx):
packager_args.add(_flatten_paths(esm5), join_with=",")
packager_args.add(_flatten_paths(bundles), join_with=",")
packager_args.add([s.path for s in ctx.files.srcs], join_with=",")
packager_args.add([d.path for d in ctx.files.data], join_with=",")
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is right - ctx.files.data will only receive the default outputs of the targets listed in data. for example, if you put a ts_library in the data (a strange thing to do) I think you'll find that the .d.ts files go in the bundle but not the .js files.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should it be all outputs?

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe add a TODO here to study the right way to gather the runfiles providers from the transitive closure, looking at the mpm_package rule as an example

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

packages/bazel/src/ng_package/packager.ts Outdated
// We want the relative path from the given file to its ancestor "root" directory.
// This root depends on whether the file lives in the source tree (srcDir) as a basic file
// input to ng_package, or in the out tree (binDir) as the output of another rule.
const rootDir = inputPath.includes(binDir) ? binDir : srcDir;
Copy link
Contributor

Choose a reason for hiding this comment

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

should check genfilesDir too

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@mary-poppins
Copy link

You can preview 51dbe69 at https://pr22919-51dbe69.ngbuilds.io/.

alfaproject
alfaproject reviewed Mar 22, 2018
View reviewed changes
packages/bazel/src/ng_package/packager.ts Outdated
@@ -14,6 +14,9 @@ function main(args: string[]): number {
// Exit immediately when encountering an error.
shx.set('-e');

// Keep track of whether an error has occured so that we can return an appropriate exist code.
Copy link
Contributor

Choose a reason for hiding this comment

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

exit

@matsko matsko added the area: build & ci Related the build and CI infrastructure of the project label Mar 22, 2018
@mary-poppins
Copy link

You can preview 318834a at https://pr22919-318834a.ngbuilds.io/.

@mary-poppins
Copy link

You can preview c1f6225 at https://pr22919-c1f6225.ngbuilds.io/.

@mary-poppins
Copy link

You can preview e2ffeed at https://pr22919-e2ffeed.ngbuilds.io/.

@jelbourn jelbourn force-pushed the ng_package_data branch 2 times, most recently from ab3cdb1 to fa09def Compare March 22, 2018 18:35
@mary-poppins
Copy link

You can preview ab3cdb1 at https://pr22919-ab3cdb1.ngbuilds.io/.

@mary-poppins
Copy link

You can preview fa09def at https://pr22919-fa09def.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 841bc36 at https://pr22919-841bc36.ngbuilds.io/.

@jelbourn jelbourn added the action: merge The PR is ready for merge by the caretaker label Mar 22, 2018
@ngbot
Copy link

ngbot bot commented Mar 22, 2018

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure missing required label: "PR target: *"
    pending status "google3" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@jelbourn jelbourn added the target: major This PR is targeted for the next major release label Mar 22, 2018
@jelbourn
Copy link
Member Author

For caretaker: this PR doesn't affect g3

@matsko matsko closed this in d9a0a8f Mar 23, 2018
@mary-poppins
Copy link

You can preview 41749db at https://pr22919-41749db.ngbuilds.io/.

leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018
@jelbourn @leo6104
build(bazel): add data attr to ng_package (angular#22919)
bca26ae 
PR Close angular#22919
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alfaproject alfaproject alfaproject left review comments

@alexeagle alexeagle alexeagle approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: build & ci Related the build and CI infrastructure of the project cla: yes hotlist: components team Related to Angular CDK or Angular Material target: major This PR is targeted for the next major release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@jelbourn @mary-poppins @alexeagle @alfaproject @matsko @googlebot