Improve frontend-building related Makefile rules

[StCyr]

Currently it's necessary to run npm run sass:icons in addition to make dev-setup and make build-js, to have the icons on your development server.

This PR improves the frontend-building related Makefile rules to make this additional manual action unneeded anymore

Signed-off-by: Cyrille Bollu [email protected]

[StCyr]

I don't think the drone failure is due to my changes

[skjnldsv]

I think we should add the sass commands as a postbuild step instead ? Makefiles are arbitrary and not ideal imho

[StCyr]

I think we should add the sass commands as a postbuild step instead ? Makefiles are arbitrary and not ideal imho

I don't understand your comment :-(

[skjnldsv]

I don't understand your comment :-(

https://docs.npmjs.com/cli/v8/using-npm/scripts#pre--post-scripts

  "scripts": {
    "build": "NODE_ENV=production webpack --progress --config webpack.prod.js",
    "postbuild": "npm run sass && npm run sass:icons",
    "sass": "sass --load-path core/css core/css/ apps/*/css",
    "sass:icons": "babel-node core/src/icons.js"
  }

[PVince81]

I've lost quite some time believing this was already merged and kept compiling and trying different node versions. I should have looked at the exact failure to see that it was always about icons.

Anyway, we should push this forward fast because others might bump into the same issues.

[PVince81]

I've rebased this and added the post-build step

next up:

• [x] local test

[PVince81]

tested locally, while it does run those commands, there's another separate issue: in README we tell people to run "make build-js-production" and now that would include those commands.

however the scss commands are also exploring the non-core apps and displaying errors for me locally, so nothing gets built. and this happens right at the end of the time-consuming build. should we make it a pre-build step instead ?

I think we should also limit the scss commands to the apps that are part of the core repo. Otherwise we need to state in the README that for buildling production assets one needs to delete all non-core apps.

Thoughts ?

[PVince81]

for reference, this is the local output on my setup:

%  npm run sass && npm run sass:icons

> [email protected] sass
> sass --load-path core/css core/css/ apps/*/css

Error: Undefined mixin.
   ╷
26 │ @include icon-black-white('briefcase', 'calendar', 5);
   │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   ╵
  apps/calendar/css/icons.scss 26:1  root stylesheet

Error: Undefined variable.
   ╷
38 │         margin-bottom: $grid-height-unit;
   │                        ^^^^^^^^^^^^^^^^^
   ╵
  apps/contacts/css/Properties/Properties.scss 38:18  root stylesheet

Error: Undefined mixin.
  ╷
1 │ @include icon-black-white('deck', 'deck', 1);
  │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵
  apps/deck/css/deck.scss 1:1  root stylesheet

Error: Undefined mixin.
  ╷
4 │ @include icon-black-white('deck', 'deck', 1);
  │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵
  apps/deck/css/icons.scss 4:1          @import
  apps/deck/css/globalstyles.scss 27:9  root stylesheet

Error: Undefined mixin.
  ╷
4 │ @include icon-black-white('deck', 'deck', 1);
  │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵
  apps/deck/css/icons.scss 4:1  root stylesheet

Error: Undefined mixin.
    ╷
308 │ @include icon-black-white('inbox', 'mail', 1);
    │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    ╵
  apps/mail/css/mail.scss 308:1  root stylesheet

Error: Undefined variable.
  ╷
1 │ @media only screen and (max-width: $breakpoint-mobile) {
  │                                    ^^^^^^^^^^^^^^^^^^
  ╵
  apps/mail/css/mobile.scss 1:36  root stylesheet
Deprecation Warning: Using / for division outside of calc() is deprecated and will be removed in Dart Sass 2.0.0.

Recommendation: math.div($size, 210) or calc($size / 210)

More info and automated migrator: https://sass-lang.com/d/slash-div

   ╷
68 │             $sizeY: $size / 210 * 297;
   │                     ^^^^^^^^^^^
   ╵
    apps/officeonline/css/admin.scss 68:12  root stylesheet


Error: Undefined mixin.
   ╷
24 │     @include icon-color('folder', 'filetypes', $color-black, 1, true);
   │     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   ╵
  apps/photos/css/icons.scss 24:2  root stylesheet

Error: Undefined variable.
   ╷
62 │ @media only screen and (max-width: $breakpoint-mobile) {
   │                                    ^^^^^^^^^^^^^^^^^^
   ╵
  apps/serverinfo/css/style.scss 62:36  root stylesheet

Error: Undefined mixin.
  ╷
1 │ @include icon-black-white('reply', 'spreed', 1);
  │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵
  apps/spreed/css/icons.scss 1:1  root stylesheet

Error: Undefined mixin.
  ╷
1 │ @include icon-black-white('reply', 'spreed', 1);
  │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵
  apps/spreed/css/icons.scss 1:1         @import
  apps/spreed/css/merged-files.scss 2:9  root stylesheet

Error: Undefined mixin.
  ╷
1 │ @include icon-black-white('reply', 'spreed', 1);
  │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵
  apps/spreed/css/icons.scss 1:1                @import
  apps/spreed/css/merged-public-share.scss 2:9  root stylesheet

Error: Undefined mixin.
  ╷
1 │ @include icon-black-white('reply', 'spreed', 1);
  │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵
  apps/spreed/css/icons.scss 1:1              @import
  apps/spreed/css/merged-share-auth.scss 2:9  root stylesheet

Error: Undefined mixin.
  ╷
1 │ @include icon-black-white('reply', 'spreed', 1);
  │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵
  apps/spreed/css/icons.scss 1:1   @import
  apps/spreed/css/merged.scss 2:9  root stylesheet

Error: Undefined variable.
   ╷
30 │         border-color: $color-error;
   │                       ^^^^^^^^^^^^
   ╵
  apps/spreed/css/settings-admin.scss 30:17  root stylesheet

Error: Undefined variable.
    ╷
131 │     border-color: $color-error;
    │                   ^^^^^^^^^^^^
    ╵
  apps/support/css/support.scss 131:16  root stylesheet

Error: Undefined mixin.
  ╷
1 │ @include icon-black-white('tasks', 'tasks', 1);
  │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵
  apps/tasks/css/tasks-talk.scss 1:1  root stylesheet

[StCyr]

tested locally, while it does run those commands, there's another separate issue: in README we tell people to run "make build-js-production" and now that would include those commands.

however the scss commands are also exploring the non-core apps and displaying errors for me locally, so nothing gets built. and this happens right at the end of the time-consuming build. should we make it a pre-build step instead ?

I think we should also limit the scss commands to the apps that are part of the core repo. Otherwise we need to state in the README that for buildling production assets one needs to delete all non-core apps.

Thoughts ?

Ah, maybe you've find out why this command has never been included in the building scripts in the first place; It looks quite difficult to solve efficiently (ie: maintaining a list of core apps).

If we are to update the README file, I would prefer to remove the command from the build script altogether and update the README file with something along the line:

--- a/README.md
+++ b/README.md
@@ -74,6 +74,9 @@ make watch-js
 
 # build for production with minification
 make build-js-production
+
+# build icons (caution: will also try to build icons for non-core apps)
+make icons

And create the corresponding Makefile rule.

[skjnldsv]

for reference, this is the local output on my setup:

Yes, because you are cloning foreign apps into your server/apps folder Use apps2 or use a clean server repo :)

[PVince81]

for reference, this is the local output on my setup:

Yes, because you are cloning foreign apps into your server/apps folder Use apps2 or use a clean server repo :)

yes, I've switched to "apps-extra" since, maybe need to add a note about this in the README so other people don't get surprised

[PVince81]

closing in favor of https://github.com/nextcloud/server/pull/35622 which uses pure npm approach of post-build