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 build #90

Merged
merged 30 commits into from
Jul 15, 2021
Merged

Fix build #90

merged 30 commits into from
Jul 15, 2021

Conversation

boeckMt
Copy link
Member

@boeckMt boeckMt commented Jun 24, 2021

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

GitHub Actions is currently not publishing any packages to npm because they were built with ng build --prod=true which looks as it is not working correctly anymore since the change to "builder": "@angular-devkit/build-angular:ng-packagr".

See the workflow run https://github.com/dlr-eoc/ukis-frontend-libraries/runs/2903479305?check_suite_focus=true npm publish... for more details.

Found out that the original problem for the issue was NGCC running over the compiled packages #90 (comment) !

and Issue Number:

What is the new behavior?

  • NGCC was removed from the postinstall script. This corrected the build without compiling with NGCC so the script "scripts":{"prepublishOnly":"node --eval \"console.error('ERROR: Trying to publish a package that has been compiled by NGCC. This is not allowed.\\nPlease delete and rebuild the package, without compiling with NGCC, before attempting to publish.\\nNote that NGCC may have been run by importing this package into another project that is being built with Ivy enabled.\\n')\" && exit 1"}} is not added anymore to the packages.

  • Further Issue Correct options for depcheck #87 is fixed so depcheck should work correctly now.

  • The workspace files generated from angular where recreated, so we don't use any old configuration c0b5d81

  • Add Types for node again to the packages which need them for test and build 532b73b

  • Update angular to latest version before 12.x

  • Fixes Problem 2 Fix build #90 (comment)

Does this PR introduce a breaking change?

  • Yes
  • No

@boeckMt
Copy link
Member Author

boeckMt commented Jun 24, 2021

Problem 1
npm run build is using NGCC somehow:

  • Unfortunately the build still don't works like it is working on the local machine.

  • I tried to change the package look with npm v6.14.13 because of the following warning in the build npm WARN read-shrinkwrap This version of npm is compatible with lockfileVersion@1, but package-lock.json was generated for lockfileVersion@2. I'll try to do my best with it! but this is also not working.

  • Maybe it is something with the npm cache in the GitHub runner workspace?

Problem 2
The problem why jobs not failing on publish:

  • run: find ./dist -mindepth 1 -maxdepth 1 -type d -exec bash -c "cd '{}' && npm run testexit" \ // exit codes of the subshells executed by find are not passed to the main shell.

@boeckMt boeckMt marked this pull request as draft June 24, 2021 15:44
@boeckMt boeckMt removed the request for review from MichaelLangbein June 24, 2021 15:45
@boeckMt
Copy link
Member Author

boeckMt commented Jul 8, 2021

Problem 1
npm run build is using NGCC somehow:

angular/angular#35762 -> This is happening on old library tsconfigs with

...
"angularCompilerOptions": {
   "enableIvy": false
 }
...

when running ngcc in our postinstall script, which unfortunately runs over the the downloaded artifact from the build job.

The compiler option

...
"angularCompilerOptions": {
   "compilationMode": "partial"
 }
...

which removes this behaviour is available in angular@12 so maybe remove ngcc from the postinstall till we can update to the new version.

@boeckMt boeckMt marked this pull request as ready for review July 8, 2021 13:17
Copy link
Collaborator

@MichaelLangbein MichaelLangbein left a comment

Choose a reason for hiding this comment

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

Alright, that must have been a lot of work - thanks!
I did leave some comments, but they are only for clarification and fixing some typo's - so this PR is being approved immediately. @boeckMt : I'll leave merging to you, in case you want to add some docs about the comments in this review ... but honestly, I think that's optional, because build-processes change quickly anyway.

# You can see what browsers were selected by your queries by running:
# npx browserslist

> 0.5%
last 2 versions
last 1 Chrome version
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we deliberately test only with latest Chrome, but with the two latest Safaris?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was generated by angular, I only want to update the File because we did not change anything on it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Roger that! No further questions.

@@ -14,7 +14,7 @@ jobs:
- uses: actions/checkout@v2
- uses: actions/setup-node@v2
with:
node-version: 14
node-version: 15
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment is here only for documentation-purposes: a few days ago we have decided to build against the newest version of node, which might not always be the LTS. You're welcome, future generations!

Copy link
Member Author

@boeckMt boeckMt Jul 14, 2021

Choose a reason for hiding this comment

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

To switch to this version was only a test, because the build stopped working somehow here eccac77 where only patch versions are changed. Thought it could be something with the cache of actions/setup-node@v2. At first I tried the newest node version 16 but it didn't work either. It looked like a problem with node-sass and node but it also doesn't worked after removal.

On the next week I used node 15 and it suddenly worked... I hope GitHub Actions will get better to debug in the future :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm! Well, whatever works. Thanks for the heads up!

@@ -1,13 +1,19 @@
// This file is required by karma.conf.js and loads recursively all the .spec and framework files

import 'zone.js/dist/zone';
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are several cases where we're importing zone alongside testing-zone for our unit-tests, but I don't see changes to the actual tests. Is this a CI requirement? Is it because we're no longer using karma-coverage-istanbul? Or because of those new async methods in the e2e tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

Also generated by angular.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks!

It uses the package scope `@dlr-eoc`, `main package.json` and the angular config file `angular.json`.


- To get the list of all projects is reads the the angular config file (angular.json).
Copy link
Collaborator

Choose a reason for hiding this comment

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

To get the list of all projects it reads the angular config file (angular.json).

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 9fb3013.


- To get the list of all projects is reads the the angular config file (angular.json).
- To check all dependencies it uses the package scope `@dlr-eoc` and `depcheck`.
- To get the build order it used `depcheck` and `toposort`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

To get the build order it uses depcheck and toposort.

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 57fd125.

@boeckMt
Copy link
Member Author

boeckMt commented Jul 14, 2021

Thanks for the review!

I have a few changes left, to adjust the workflow and ci scripts to fix Problem 2 -> #90 (comment).
You can check this later on.

@boeckMt
Copy link
Member Author

boeckMt commented Jul 14, 2021

A new error occurred on npm publish to npm registry in the action with npm version 7.7.6 from actions/setup-node@v2 node v15.14.0. Old version on the last working publish was npm 6.14.13 and node v14.17.0.

npm ERR! need auth You need to authorize this machine using `npm adduser`

@boeckMt
Copy link
Member Author

boeckMt commented Jul 15, 2021

Now it finally worked :)
https://github.com/dlr-eoc/ukis-frontend-libraries/actions/runs/1033733022 (Publish to GPR is failing here because it worked before)

@boeckMt boeckMt merged commit 1c43e0b into master Jul 15, 2021
@boeckMt boeckMt deleted the fix-build branch July 15, 2021 12:23
@boeckMt boeckMt mentioned this pull request Dec 13, 2021
14 tasks
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.

None yet

2 participants