Skip to content

build: build cdk with angular from source #13541

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
Merged
merged 2 commits into from
Oct 11, 2018
Merged

build: build cdk with angular from source #13541

merged 2 commits into from
Oct 11, 2018

Conversation

devversion
Copy link
Member

@devversion devversion commented Oct 10, 2018 •
edited
Loading

  • Continues the long-term Bazel goal by fixing the CDK bazel build
    targets. Also builds the CDK targets with Angular from source.

Follow up's will try to make serving, and testing possible.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 10, 2018
@devversion devversion added blocked This issue is blocked by some external factor, such as a prerequisite PR pr: merge safe target: major This PR is targeted for the next major release labels Oct 10, 2018
@devversion
build: build cdk with angular from source
1c63012 
* Continues the long-term Bazel goal by fixing the CDK bazel build
targets. Also builds the CDK targets with Angular from source.

Follow up's will try to make serving, and testing possible.
@devversion devversion removed the blocked This issue is blocked by some external factor, such as a prerequisite PR label Oct 10, 2018
@devversion devversion force-pushed the build/build-with-bazel branch from f66f136 to 1c63012 Compare October 10, 2018 18:28
jelbourn
jelbourn reviewed Oct 10, 2018
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

This PR should also re-enable these passing targets on CirceCI

WORKSPACE
http_archive(
name = "angular",
# Temporarily locked down to the angular/angular:bazel branch. This branch includes necessary
# commits that make building from source possible.
Copy link
Member

Choose a reason for hiding this comment

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

Is there an issue somewhere tracking this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not that I know of. I think it's just a WIP branch that includes the new design with rules_angular_dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

@alexeagle what's the status of the changes on that branch making it to angular master?

src/cdk/coercion/BUILD.bazel

ng_module(
ts_library(
Copy link
Member

Choose a reason for hiding this comment

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

I believe that the ng_package rule only worked with this being an ng_module, but maybe that is moot now since we can't package the library without ivy anyway

Copy link
Member Author

Choose a reason for hiding this comment

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

Just checked locally, and seems to work as expected.

@devversion
Build package on CI
22cbdef 
* No longer compile with AOT compiler that generates factory files that cannot be packaged into "ng_package"
@devversion devversion force-pushed the build/build-with-bazel branch from 334f1fc to 22cbdef Compare October 10, 2018 20:30
@devversion
Copy link
Member Author

devversion commented Oct 10, 2018 •
edited
Loading

@jelbourn The CI now builds the CDK npm package. There are a few source map errors coming from SaSS. I will try to resolve those in a follow-up, as well as the warning about the "module metadata" if that works for you.

Since the ng_package does not work if we compile using AOT, I had to use the JIT ngtsc compiler strategy. I think for our development needs (serving & testing), we should rather use AOT and ignore the ng_package for now. The goal of this PR is just the "successful" building for now.

jelbourn
jelbourn approved these changes Oct 10, 2018
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

That works for me. Another approach you could take is to add tags = ["manual"], to the rule so that something like bazel build ... won't run it.

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Oct 10, 2018
@alexeagle
Copy link
Contributor

Thanks @devversion !

@josephperrott josephperrott removed their request for review October 11, 2018 15:06
@andrewseguin andrewseguin merged commit 282736f into angular:master Oct 11, 2018
roboshoes pushed a commit to roboshoes/material2 that referenced this pull request Oct 23, 2018
@devversion @roboshoes
build: build cdk with angular from source (angular#13541)
8719803 
* build: build cdk with angular from source

* Continues the long-term Bazel goal by fixing the CDK bazel build
targets. Also builds the CDK targets with Angular from source.

Follow up's will try to make serving, and testing possible.

* Build package on CI

* No longer compile with AOT compiler that generates factory files that cannot be packaged into "ng_package"
@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 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jelbourn jelbourn jelbourn approved these changes

@andrewseguin andrewseguin Awaiting requested review from andrewseguin

@crisbeto crisbeto Awaiting requested review from crisbeto

@mmalerba mmalerba Awaiting requested review from mmalerba

Assignees

@jelbourn jelbourn

Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement 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.
5 participants
@devversion @alexeagle @jelbourn @googlebot @andrewseguin