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

LPS-128362 - Enable dartSass build option for themes #843

Closed

Conversation

bryceosterhaus
Copy link
Collaborator

This is not intended to be merged until @liferay/npm-scripts is merged with a greater version than v37.1.2

This PR is intended to be used with liferay-theme-tasksv10.3.0 which is included in the next npm-scripts release.

I am sending this now as a draft now in case I am on leave when npm-scripts is updated. Feel free to test and merge once npm-scripts is updated.

@liferay-continuous-integration
Copy link
Collaborator

CI is automatically triggering the following test suites:

  •     ci:test:relevant
  •     ci:test:sf

@bryceosterhaus
Copy link
Collaborator Author

ci:stop

@liferay-continuous-integration
Copy link
Collaborator

❌ ci:test:sf - 0 out of 1 jobs passed in 4 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: bd5c21572d7e5a65df27350fbed9c35997ed1375

Sender Branch:

Branch Name: LPS-128362
Branch GIT ID: d701a2606942e1a882852fbf6a958c5b241d76ed

0 out of 1jobs PASSED
For more details click here.
     [java] java.lang.Exception: Found 1 formatting issues:
     [java] 1: ./modules/apps/frontend-theme/frontend-theme-admin/package.json expected:<...: "admin-theme",
     [java] 		"[rubySass": false,
     [java] 		"sassOptions": \{
     [java] 			"dartSass": true
     [java] 		\}],
     [java] 		"version": "7.1"...> but was:<...: "admin-theme",
     [java] 		"[sassOptions": \{
     [java] 			"dartSass": true
     [java] 		\},
     [java] 		"rubySass": false],
     [java] 		"version": "7.1"...>
     [java] 
     [java] 	at com.liferay.source.formatter.SourceFormatter.format(SourceFormatter.java:445)
     [java] 	at com.liferay.source.formatter.SourceFormatter.main(SourceFormatter.java:290)
[stopwatch] [run.batch.test.action: 1:52.026 sec]
     [echo] The following error occurred while executing this line:
     [echo] /opt/dev/projects/github/liferay-portal/portal-impl/build.xml:572: The following error occurred while executing this line:
     [echo] /opt/dev/projects/github/liferay-portal/portal-impl/build.xml:706: Java returned: 1
      [get] Getting: http://test-1-14/job/test-portal-source-format/4610//consoleText
      [get] To: /opt/dev/projects/github/liferay-portal/20210225142457385.txt
   [delete] Deleting: /opt/dev/projects/github/liferay-portal/20210225142457385.txt
  [typedef] Could not load definitions from resource org/apache/maven/artifact/ant/antlib.xml. It could not be found.
  [taskdef] Could not load definitions from resource org/jacoco/ant/antlib.xml. It could not be found.
   [delete] Deleting: /opt/dev/projects/github/liferay-portal/null2145355187.properties

merge-test-results:
[mkdir] Created dir: /opt/dev/projects/github/liferay-portal/test-results
[junitreport] Processing /opt/dev/projects/github/liferay-portal/test-results/TESTS-TestSuites.xml to /tmp/null1256129401
[junitreport] Loading stylesheet jar:file:/opt/java/ant/lib/ant-junit.jar!/org/apache/tools/ant/taskdefs/optional/junit/xsl/junit-frames.xsl
[junitreport] Transform time: 242ms
[junitreport] Deleting: /tmp/null1256129401
[echo] A report with all the test results can be found at test-results/html/index.html.
[mkdir] Created dir: /opt/dev/projects/github/liferay-jenkins-ee/test-results
[copy] Copying 1 file to /opt/dev/projects/github/liferay-jenkins-ee/test-results
[echo] run.batch.test.tear.down.start.timestamp: 02-25-2021 14:25:03:549 PST
[stopwatch] [run.batch.test.tear.down: 0.001 sec]
[echo]
[echo] Ant GC log:
[echo]

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:stable - 9 out of 9 jobs passed

✔️ ci:test:relevant - 22 out of 23 jobs passed in 0 ms

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: bd5c21572d7e5a65df27350fbed9c35997ed1375

Upstream Comparison:

Branch GIT ID: 227cd0e0534215b94fa21ec5d57817752506d05b
Jenkins Build URL: Acceptance Upstream DXP (master) #1580

ci:test:stable - 9 out of 9 jobs PASSED
9 Successful Jobs:
ci:test:relevant - 21 out of 23 jobs PASSED
21 Successful Jobs:
For more details click here.

This pull contains no unique failures.


Failures in common with acceptance upstream results at 227cd0e:

@liferay-continuous-integration
Copy link
Collaborator

@liferay-continuous-integration
Copy link
Collaborator

@wincent
Copy link

wincent commented Feb 26, 2021

Changes look good. I added the "on hold" label until the release is out and integrated, at which point we can test.

@jbalsas
Copy link

jbalsas commented Mar 5, 2021

Testing this as we speak

@jbalsas
Copy link

jbalsas commented Mar 5, 2021

Ok, so this seems to work other than it basically doubles compilation time for themes 🙈

I think we have no other option since node-sass is dead, but... 🤔

@wincent, thoughts?

@wincent
Copy link

wincent commented Mar 5, 2021

Ok, so this seems to work other than it basically doubles compilation time for themes 🙈

I think we have no other option since node-sass is dead, but... 🤔

@wincent, thoughts?

Previously discussed here. We always knew it was going to be slower, but hopefully not by too much relative to our overall build process, and hopefully not for ever. What were the concrete numbers you saw?

But at the end of the day, our hand is forced here by the fact that node-sass is a dead-end. Look on the bright side: it's a step closer to seeing the end of support requests related to node-sass exploding whenever it sees a different version of Node.

@jbalsas
Copy link

jbalsas commented Mar 5, 2021

K, I rebased, fixed SF and force-pushed.

What were the concrete numbers you saw?

I was seeing below 20s for node-sass and above 30s for dart-sass. Funny part of this is we're already using renderSync which as per Sass propaganda is twice as fast as the other option.

Note however that by default, renderSync() is more than twice as fast as render() due to the overhead of asynchronous callbacks. To avoid this performance hit, render() can use the fibers package to call asynchronous importers from the synchronous code path. To enable this, pass the Fiber class to the fiber option

I don't want to even try and see how slow this gets with renderSync... 🙈

Seeing as this is what it is, going to take it out of draft and give forward a try 🤞

@jbalsas jbalsas marked this pull request as ready for review March 5, 2021 17:33
@jbalsas
Copy link

jbalsas commented Mar 5, 2021

ci:forward

@liferay-continuous-integration
Copy link
Collaborator

CI is automatically triggering the following test suites:

  •     ci:test:relevant
  •     ci:test:sf

The pull request will automatically be forwarded to the user brianchandotcom if the following test suites pass:

  •     ci:test:relevant
  •     ci:test:sf
  •     ci:test:stable

@jbalsas
Copy link

jbalsas commented Mar 5, 2021

I don't want to even try and see how slow this gets with renderSync... 🙈

Alas, I tried and it's as bad as I expected 😂

@liferay-continuous-integration
Copy link
Collaborator

❌ ci:test:stable - 8 out of 9 jobs passed

❌ ci:test:relevant - 18 out of 24 jobs passed in 4 hours 6 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: 97c81548cc2c7aa05bc0f2a815c8d30b26549845

Upstream Comparison:

Branch GIT ID: 97c81548cc2c7aa05bc0f2a815c8d30b26549845
Jenkins Build URL: Acceptance Upstream DXP (master) #1609

ci:test:stable - 8 out of 9 jobs PASSED
8 Successful Jobs:
ci:test:relevant - 18 out of 24 jobs PASSED
18 Successful Jobs:
For more details click here.

Failures unique to this pull:


Failures in common with acceptance upstream results at 97c8154:
  1. test-portal-acceptance-pullrequest-batch(master)/functional-tomcat90-mysql57-jdk8/0
    Job Results:

    42 Tests Passed.
    18 Tests Failed.

    1. ...

@liferay-continuous-integration
Copy link
Collaborator

@jbalsas
Copy link

jbalsas commented Mar 8, 2021

Let's try this one more time...

@jbalsas
Copy link

jbalsas commented Mar 8, 2021

ci:forward

@liferay-continuous-integration
Copy link
Collaborator

CI is automatically triggering the following test suites:

  •     ci:test:relevant
  •     ci:test:sf

The pull request will automatically be forwarded to the user brianchandotcom if the following test suites pass:

  •     ci:test:relevant
  •     ci:test:sf
  •     ci:test:stable

@liferay-continuous-integration
Copy link
Collaborator

Skipping previously passed test suites:
ci:test:sf

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:stable - 9 out of 9 jobs passed

✔️ ci:test:relevant - 22 out of 24 jobs passed in 3 hours 24 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: 7426a6a739b6ca42f7d8ae55a88aa1b0c9112dba

Upstream Comparison:

Branch GIT ID: 7426a6a739b6ca42f7d8ae55a88aa1b0c9112dba
Jenkins Build URL: Acceptance Upstream DXP (master) #1610

ci:test:stable - 9 out of 9 jobs PASSED
9 Successful Jobs:
ci:test:relevant - 21 out of 24 jobs PASSED
21 Successful Jobs:
For more details click here.

This pull contains no unique failures.


Failures in common with acceptance upstream results at 7426a6a:
  1. test-portal-acceptance-pullrequest-batch(master)/functional-tomcat90-mysql57-jdk8/0
    Job Results:

    42 Tests Passed.
    18 Tests Failed.

    1. ...

@liferay-continuous-integration
Copy link
Collaborator

All required test suite(s) passed.
Forwarding pullrequest to brianchandotcom.

@liferay-continuous-integration
Copy link
Collaborator

Pull request has been successfully forwarded to brianchandotcom#99625

@liferay-continuous-integration
Copy link
Collaborator

@pat270
Copy link

pat270 commented Mar 8, 2021

@jbalsas @wincent @bryceosterhaus Maybe node-sass isn't down for the count, there is active development in libSass. See sass/libsass#3135.

@jbalsas
Copy link

jbalsas commented Mar 8, 2021

@jbalsas @wincent @bryceosterhaus Maybe node-sass isn't down for the count, there is active development in libSass. See sass/libsass#3135.

wat

@wincent
Copy link

wincent commented Mar 8, 2021

Although they still have the huge deprecation notice on the readme:

Warning: LibSass is deprecated. While it will continue to receive maintenance releases indefinitely, there are no plans to add additional features or compatibility with any new CSS or Sass features. Projects that still use it should move onto Dart Sass.

which they added as part of the pretty decisive-sounding PR #3133 (related issue: #3123).

One of the maintainers may have had a change of heart, but I am still inclined to think fool me once, shame on... shame on you... but fool me you can't get fooled again.

@pat270
Copy link

pat270 commented Mar 8, 2021

Haha 👍 @wincent

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants