[4.0] use sass-embedded, reducing build time by 20-30% #33170
Conversation
joomla-cms-bot
added
NPM Resource Changed
|
@richard67 do you know if the drone server runs on a 32bit linux? |
|
you can check with node, add to your script https://nodejs.org/api/process.html#process_process_arch but I doubt that it is 32, no one should use it nowadays for linux |
|
@Fedik it should be a shell or env variables problem (the script actually has x86 32bit binaries). But it uses some bash command (that probably is the one failing here): #!/bin/sh
# This script drives the standalone sass_embedded package, which bundles together a
# Dart executable and a snapshot of sass_embedded.
follow_links() {
file="$1"
while [ -h "$file" ]; do
# On Mac OS, readlink -f doesn't work.
file="$(readlink "$file")"
done
echo "$file"
}
# Unlike $0, $BASH_SOURCE points to the absolute path of this file.
path=`dirname "$(follow_links "$0")"`
exec "$path/src/dart" "$path/src/dart-sass-embedded.snapshot" "$@" |
|
@dgrammatiko The npm build fails in drone, see https://ci.joomla.org/joomla/joomla-cms/42050/1/7 : |
|
@richard67 I know the error is |
You mean it works for you locally? |
|
Yes, I wouldn't really create a PR with non-working code... |
@dgrammatiko I haven't assumed that. Here it works too locally on Ubuntu. Other thing: I think you should change your testing instructions from "npm install" to "npm ci". |
Done but it's also worthless if the CI is failing, thus the PR can't be merged |
Well, we should try to fix that. Regarding 32 or 64 bit of our testing environment I have no idea, but it is not sooooo uncommon to still have 32 bit compilations running on 64 bit systems. @Hackwar Do you know if our Drone runs on 32 or 64 bit Linux? |
|
I think the error comes from their postinstall script: > sass-embedded@1.0.0-beta.0 postinstall /********/src/node_modules/sass-embedded
11 > node ./download-compiler-for-end-user.js
12
13 Downloading dart-sass-embedded release asset.
14 Unzipping dart-sass-embedded release asset to ./dist/lib/src/vendor.The last line indicates that the script is attempting to d/l the executable on a relative (to the node_modules/sass_...) and obviously there's a path conflict there. |
|
@dgrammatiko So here it works on Linux and fails on Windows with the same error as we have in drone. |
|
@dgrammatiko Node version on my Linux where it works is v12.13.0, on my Windows where it fails it's v14.15.5. So it's maybe related to node versions? |
|
@richard67 might be but it works on my v14: |
|
@dgrammatiko I think I have found the mistake in their build script. Will see if I can make PR there and let you know when done. |
|
@dgrammatiko sass/embedded-host-node#45 ... let's see what happens. |
|
@dgrammatiko Hmm, seems that was not all. Drone is still failing. I've restarted it to be sure, but I'm not optimistic. |
|
I've spent the last hour trying to build it locally but without any success |
|
Here "npm ci" works fine on Linux (like before) and Windows (now after the last change for the update). I have no idea what goes wrong with the "npm" step in Drone. |
dgrammatiko
force-pushed
the
—sass-embeded—4.0-dev
branch
from
ebf1606
to
68534aa
Compare
|
our CI uses Debian buster 64bit base systems with docker images. Depending on the task we have our own images inherited from upstream or directly the main docker images provided by the relevant software package in this case NPM and we use node:14-alpine for running npm. So you should be able to reproduce the problem locally if you use the node:14-alpine docker image. |
|
@dgrammatiko Should your code be the complete content of file |
|
|
@dgrammatiko Your code is for the ".ts" file, but I need to modify the ".js". |
|
@richard67 can you run the script from the docker's shell: add Also can you try changing the line |
@dgrammatiko It's not a script, it's a binary on Linux. If I pass it as command to docker, it can't be executed because it is not found: If I pass in the same way an
Where should I add that, in which file?
Where should I do that change, in which file? |
Both are pointing in the same file:
Are we hitting the weirdness of symlinks on the containers here? |
|
I still don't get it. I don't have a symlink here anywhere below |
|
Yeah there's no symlink but I have |
|
@dgrammatiko I think it needs to sign the changed .drone.yml. |
Sure but it's not a symlink problem. Btw which of all the node-alpine packages are you using? |
|
„14-alpine“ with exactly that name. |
|
But I‘ve also tried „12-alpine“ and „15-alpine“ (the numbers are the node versions) with the same result, so it‘s not a problem with a particular version. |
|
I ran npm ci three times with an without the patch on a Macbook Pro. The real time difference was about 7 seconds in 90 seconds. Without patch: 1:30, 1:31, 1:30 Timer: Build finished in 79371 ms added 942 packages, and audited 943 packages in 2m 2 packages are looking for funding found 0 vulnerabilities === With patch: 1:25, 1:23, 1:22 added 949 packages, and audited 950 packages in 1m 8 vulnerabilities (1 moderate, 7 high) To address issues that do not require attention, run: To address all issues (including breaking changes), run: Run == did npm audit fix --force then npm ci with the following error: events.js:292 Error: spawn /Users/ceford/Sites/j4test/node_modules/sass-embedded/dist/lib/src/vendor/dart-sass-embedded/dart-sass-embedded ENOENT npm ERR! A complete log of this run can be found in: This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33170. |
before: |
…eded—4.0-dev # Conflicts: # package-lock.json # package.json
|
@HLeithner could you approve the drone changes here? |
|
@dgrammatiko approved, are you planing other changes to the drone file? |
|
This PR will never work correctly in the Drone CI if the Node container doesn't have the git app (apt-get install git) and since I have no clue how the whole CI was assembled I can't fix it... Anyone interested to move this forward could copy the changes from here and patch the missing git from the Node Alpine container |
Pull Request for Issue # .
Summary of Changes
sass-embeddedpackage instead of the dart-sass (it's still dart sass under the hood, but that's the technical part)Testing Instructions
Run
npm ciand compare the build timesActual result BEFORE applying this Pull Request
Expected result AFTER applying this Pull Request
Documentation Changes Required
No, packages are supposed to be easily interchangeable