After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 792822 - data: Automatically generate CSS files out of the SASS sources
data: Automatically generate CSS files out of the SASS sources
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2018-01-23 12:47 UTC by Mario Sánchez Prada
Modified: 2018-02-13 11:00 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
data/theme/meson.build: Fix generation of CSS files out of SCSS ones (4.57 KB, patch)
2018-01-23 13:04 UTC, Mario Sánchez Prada
none Details | Review
data/theme: Remove CSS files now automatically generated from SASS sources (100.18 KB, patch)
2018-01-23 13:04 UTC, Mario Sánchez Prada
none Details | Review
data/theme/meson.build: Fix generation of CSS files out of SCSS ones (103.09 KB, patch)
2018-01-23 16:11 UTC, Mario Sánchez Prada
none Details | Review
build: Make sassc mandatory and always generate CSS files from Sass sources (102.30 KB, patch)
2018-02-07 12:45 UTC, Mario Sánchez Prada
committed Details | Review

Description Mario Sánchez Prada 2018-01-23 12:47:11 UTC
While rebasing our (Endless) fork of GNOME Shell on top of 3.26, I found out (with the help of ebassi) that the meson rules currently in place to generate the CSS files out of the SASS sources don't quite work as expected.

As a result, and inspired by what it's done in GTK+ already[1], I've came up with the following patch that fixed the code generation step + removes the checked in CSS files that will now be automatically generated out of the SASS counterparts:

https://github.com/endlessm/gnome-shell/commit/a4bc4837d81cb7d226e061d8bcce59f48dc57ba6

I'm hoping this is something upstream might want, hence filing a bug for it.

[1] https://git.gnome.org/browse/gtk+/tree/gtk/theme/Adwaita/meson.build
Comment 1 Florian Müllner 2018-01-23 13:03:16 UTC
Mmh, it adds a build dependency on sassc, but since we switched away from ruby-sass, that may not be too bad.
Comment 2 Mario Sánchez Prada 2018-01-23 13:04:06 UTC
Created attachment 367307 [details] [review]
data/theme/meson.build: Fix generation of CSS files out of SCSS ones

The current logic does not really works since the output result of
custom_target('update-theme') is a stamp file that is not recognized
as part of the resources packef in the gresource XML in data/meson.build
by gnome.compile_resources(). As a result, this rules are not really
doing anything since they are unable to generate the CSS file out
of SCSS when any of the sources changes (or if the CSS file does not
exist in the first place, in case it hasn't been checked in the repo).

The solution to this is to follow the lead of GTK+[1] and declare one
custom_target() per file being generated (there are 2 CSS files at the
moment), instead of trying to generate both in a single rule that would
output a stamp file, using the parse_sass.sh script. By doing this, we
can now effectively get rid of the parse_sass.sh script, since all the
command line logic is now encoded inside the meson.build file.

[1] https://git.gnome.org/browse/gtk+/tree/gtk/theme/Adwaita/meson.build
Comment 3 Mario Sánchez Prada 2018-01-23 13:04:15 UTC
Created attachment 367308 [details] [review]
data/theme: Remove CSS files now automatically generated from SASS sources

After fixing the CSS generation using sassc in the previous commit, these
files are no longer needed since they will be automatically generated and
placed in the build directory on demmand. Thus, let's remove them and make
the dependency on 'sassc' mandatory from now on.
Comment 4 Mario Sánchez Prada 2018-01-23 13:05:58 UTC
(In reply to Florian Müllner from comment #1)
> Mmh, it adds a build dependency on sassc, but since we switched away from
> ruby-sass, that may not be too bad.

Yes, it does indeed. However, as you can see from the patches attached, I've split the single commit I have on Endless's repo into two patches: one that fixes the generation keeping the sassc dependency optional + another one that makes sassc required.

I've just tested it locally with a fresh build on top of gnome-shell's master branch and seems to work fine here as well. Let me know what you think, thanks!
Comment 5 Florian Müllner 2018-01-23 13:23:52 UTC
(In reply to Mario Sánchez Prada from comment #4)
> one that fixes the generation keeping the sassc dependency optional

No, that one doesn't work. The main point of the parse-sass.sh script is *not* to generate the two output files in one go, it is to generate keep the generated files in srcdir. With the first patch applied, any changes to the SASS files will correctly trigger a regeneration of the CSS, but the changes will neither be picked up by git (because they happen in the build dir) nor by the theme resource (which uses the CSS files from src dir).
Comment 6 Florian Müllner 2018-01-23 13:25:36 UTC
Review of attachment 367307 [details] [review]:

As mentioned, this doesn't really fix CSS generation, but rather breaks it completely :-)
Comment 7 Florian Müllner 2018-01-23 13:26:05 UTC
(In reply to Mario Sánchez Prada from comment #4)
> one that fixes the generation keeping the sassc dependency optional

No, that one doesn't work. The main point of the parse-sass.sh script is *not* to generate the two output files in one go, it is to generate keep the generated files in srcdir.
Comment 8 Mario Sánchez Prada 2018-01-23 14:27:48 UTC
(In reply to Florian Müllner from comment #5)
> (In reply to Mario Sánchez Prada from comment #4)
> > one that fixes the generation keeping the sassc dependency optional
> 
> No, that one doesn't work. The main point of the parse-sass.sh script is
> *not* to generate the two output files in one go, it is to generate keep the
> generated files in srcdir. With the first patch applied, any changes to the
> SASS files will correctly trigger a regeneration of the CSS, but the changes
> will neither be picked up by git (because they happen in the build dir)

Why do we need those changes to the CSS files to be picked by git if those will be *generated* now automatically whenever the SASS files change?
 
> nor by the theme resource (which uses the CSS files from src dir).

As far as I can tell from what I tested locally, with this patch doing a simple modification in the SASS files (e.g. increasing the font size) will regenerate the CSS file in the build directory (as you pointed out) and be picked cause the regeneration of the gresource file, which will include the change made as I can see by running gnome shell afterwards (which shows the increased font size).

What I found with the current situation in master, however, is that having a custom target that outputs `theme-update.stamp` (produced by the shell script) after generating the 2 CSS files (instead of having one custom target per CSS file) will cause `gnome.compile_resources()` in data/meson.build not to notice when anything changes at all (since that stamp file is not included in the gresource file, resulting in the rule being useless.

I'm CCing ebassi here in case he can add more information, as he was the one helping me figure this out.
Comment 9 Mario Sánchez Prada 2018-01-23 14:31:12 UTC
(In reply to Florian Müllner from comment #6)
> Review of attachment 367307 [details] [review] [review]:
> 
> As mentioned, this doesn't really fix CSS generation, but rather breaks it
> completely :-)

I'm lost. As far as I can tell (and I tested this thoroughly) there's no way the current situation works correctly, at least with a recent version of meson (perhaps something changed in the gnome module?), unless we make this change.

Likewise, doing this change made it work perfectly for me: CSS files get generated out of SASS files whenever something changes in the SASS files, and the end result seamlessly ends up in the gresource file and therefore picked up by the shell.

@Florian: I'd appreciate if you could test this locally (in case you didn't yet) and elaborate a bit more on "breaks it completely" if you still think it's the case, because I seriously don't see it (could be my fault, though).

PS: Adding ebassi now to CC, forgot to do it in my previous comment
Comment 10 Florian Müllner 2018-01-23 15:54:49 UTC
Note that the previous comment was *exclusively* about the first commit. The combination of *both* commits should work, so the easiest option to fix those issues is to simply get rid of the split.

But if you do two commits, then my expectation is that each work on their own, not make things work worse before fixing them.


(In reply to Mario Sánchez Prada from comment #8)
> Why do we need those changes to the CSS files to be picked by git if those
> will be *generated* now automatically whenever the SASS files change?

After applying the first patch:
 - generating CSS files from SASS is still optional
 - the existing CSS files from srcdir are used over the newly
   generated ones from builddir

That is, the CSS files shipped in git are never updated (either because sassc isn't installed in the first place, or because the files are now generated in the build dir), but those are still the files that are used in the gresource.
Comment 11 Florian Müllner 2018-01-23 16:06:28 UTC
(In reply to Mario Sánchez Prada from comment #9)
> I'm lost. As far as I can tell (and I tested this thoroughly) there's no way
> the current situation works correctly

Mmh, it actually works most of the time for me - changes to the SASS trigger regenerating the CSS, which in turns makes those files more recent than the generated resource.

But again, I'm arguing against the split commits more than against the change itself.
Comment 12 Mario Sánchez Prada 2018-01-23 16:10:52 UTC
(In reply to Florian Müllner from comment #10)
> Note that the previous comment was *exclusively* about the first commit. The
> combination of *both* commits should work, so the easiest option to fix
> those issues is to simply get rid of the split.
> 
> But if you do two commits, then my expectation is that each work on their
> own, not make things work worse before fixing them.

I think I understand you now. In my mind the purpose of the first commit was to  give you the option to keep using the checked-in CSS files in case you don't have sassc installed (since the custom target won't be run in that case), so that you can still build the shell with what's checked in.

But it's true that if sassc is installed then it would mean having the CSS outdated in the repo unless one manually picked them from the build dir and checked them in once again in the repo after rebuilding, which is not great. 
 
> (In reply to Mario Sánchez Prada from comment #8)
> > Why do we need those changes to the CSS files to be picked by git if those
> > will be *generated* now automatically whenever the SASS files change?
> 
> After applying the first patch:
>  - generating CSS files from SASS is still optional
>  - the existing CSS files from srcdir are used over the newly
>    generated ones from builddir
>
> That is, the CSS files shipped in git are never updated (either because
> sassc isn't installed in the first place, or because the files are now
> generated in the build dir), but those are still the files that are used in
> the gresource.

Correct. I'll merge the two patches in one then, as I had it initially, sorry.
Comment 13 Mario Sánchez Prada 2018-01-23 16:11:21 UTC
Created attachment 367320 [details] [review]
data/theme/meson.build: Fix generation of CSS files out of SCSS ones
Comment 14 Mario Sánchez Prada 2018-02-01 16:08:05 UTC
Ping? @Florian, let me know if you want me to move this one to gitlab and propose a MR there, thanks
Comment 15 Mario Sánchez Prada 2018-02-05 11:41:32 UTC
(In reply to Mario Sánchez Prada from comment #13)
> Created attachment 367320 [details] [review] [review]
> data/theme/meson.build: Fix generation of CSS files out of SCSS ones

Florian and me talked about this briefly during FOSDEM and I think we agreed on that we're not particularly excited about adding sassc as yet another dependency but, considering that it's not the previous pre-processor that pulled ruby in and that it's normally packaged in most distros (e.g. see [1]-[5]), it doesn't look that much of a terrible idea either.

It's important to note, though, that once this patch lands there will no longer be generated .css files in the source directory, just in the _build/ directory. This should not be an issue, but could be confusing at the beginning if you're used to that, I guess.

@Florian: My understanding is that this would be good to push now already, but I very much prefer if you could "rubber stamp" the decision before pushing, thanks!

[1] https://src.fedoraproject.org/rpms/sassc
[2] https://software.opensuse.org/package/sassc
[3] https://packages.debian.org/stretch/sassc
[4] https://packages.ubuntu.com/zesty/sassc
[5] https://www.archlinux.org/packages/community/x86_64/sassc
Comment 16 Florian Müllner 2018-02-06 00:47:26 UTC
Review of attachment 367320 [details] [review]:

In addition to the comments below, the commit message needs a major rewrite:

- subject: don't use file paths as prefixes, pick something like "build: " or "style: "

- the first paragraph is explaining very verbosely why the current setup cannot possibly
  work, except that it works quite well in practice

- also: "if the CSS file does not exist in the first place"? Our CSS files *are* checked in,
  so why mention some unrealistic theoretic condition?

- the second paragraph goes on a lot about the parse_sass.sh script and the stamp file, but
  completely misses the point why those existed in the first place (no, it had nothing to do
  with not defining one custom_target() per output file)

- "following the lead of GTK+" isn't really what we are doing, as GTK+ still ships CSS files
  (that require explicit updates to not get out-of-sync with the SASS, 

- the last paragraph finally manages to mention in an aside that the required stylesheets are
  now always generated at build time and sassc thus becomes a mandatory dependency - that's
  really the main point of the patch, not some sidenote

For reference, see https://gitlab.gnome.org/GNOME/gnome-shell-extensions/merge_requests/28 for the corresponding classic-style change.

::: data/theme/README
@@ +7,3 @@
 * Most SASS preprocessors should produce similar results, however the build system
+  integration uses sassc. You should be able to install it with `pkcon install sassc` or
+  your distribution's package manager.

Those changes don't make any sense - there's no longer any CSS that could be edited directly, and there's little point in mentioning other SASS preprocessors given that sassc is now a build dependency.

::: data/theme/meson.build
@@ +1,1 @@
+theme_deps = []

Any reason for the move?

@@ +23,3 @@
+                              ],
+                              depend_files: theme_sources,
+                              build_by_default: true)

This was part of the hack to make it work, but now that theme_deps refer to the actually generated files, you should be able to remove it
Comment 17 Mario Sánchez Prada 2018-02-07 12:45:47 UTC
Created attachment 367991 [details] [review]
build: Make sassc mandatory and always generate CSS files from Sass sources

Thanks for the review, Florian. See a new patch attached, and some comments inline below:

(In reply to Florian Müllner from comment #16)
> Review of attachment 367320 [details] [review] [review]:
> 
> In addition to the comments below, the commit message needs a major rewrite:
> [...]

Yes, I think I definitely misunderstood some things and the commit message reflects that. I updated it now, hopefully will be more accurate & useful this time.

> ::: data/theme/README
> @@ +7,3 @@
>  * Most SASS preprocessors should produce similar results, however the build
> system
> +  integration uses sassc. You should be able to install it with `pkcon
> install sassc` or
> +  your distribution's package manager.
> 
> Those changes don't make any sense - there's no longer any CSS that could be
> edited directly, and there's little point in mentioning other SASS
> preprocessors given that sassc is now a build dependency.

I've removed that paragraph entirely
 
> ::: data/theme/meson.build
> @@ +1,1 @@
> +theme_deps = []
> 
> Any reason for the move?

Not really, I just removed that change to minimize the diff now.

> @@ +23,3 @@
> +                              ],
> +                              depend_files: theme_sources,
> +                              build_by_default: true)
> 
> This was part of the hack to make it work, but now that theme_deps refer to
> the actually generated files, you should be able to remove it

I see, thanks for pointing it out. Updated the patch now
Comment 18 Florian Müllner 2018-02-09 12:35:55 UTC
I'm afraid I found an aspect of this that we haven't considered so far, so this will require more discussion.

With the generated CSS checked in, it is possible to easily try out someone's branch (e.g. from a merge request) as long as you make sure that the automatic rule doesn't revert any style changes.

However if we *require* the SASS source, this becomes harder: Core developers can push a branch to the upstream gnome-shell-sass repository and point the submodule there, but others will have to use a branch in their private namespace which requires a separate remote (that is, it doesn't just work from a MR's branch).

I was about to say that I couldn't think of a solution for that issue, but I may actually have found one during the bugzilla downtime:
https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/25
Comment 19 Mario Sánchez Prada 2018-02-09 21:11:40 UTC
Good point. I supposed I overlooked this one because in my the scenario where I've been using this approach so far, the CSS files were always ignored anyway and I only cared about modifying things in an additional `_endless.scss` file I created to apply on top of the ones from gnome-shell-sass, so I never had such a situation.

That said, now I understand better why you came up with https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/26, since that seems to solve this issue, at the same time it also will make my life easier for future rebases since I will no longer have to import the gnome-shell-sass submodule into our fork's tree. Thanks!
Comment 20 Florian Müllner 2018-02-09 22:11:16 UTC
Review of attachment 367991 [details] [review]:

LGTM now.
Comment 21 Mario Sánchez Prada 2018-02-10 08:01:05 UTC
Comment on attachment 367991 [details] [review]
build: Make sassc mandatory and always generate CSS files from Sass sources

Thanks for the review. Landed on master:
https://gitlab.gnome.org/GNOME/gnome-shell/commit/c62e7a6a
Comment 22 Mario Sánchez Prada 2018-02-10 08:02:49 UTC
Comment on attachment 367991 [details] [review]
build: Make sassc mandatory and always generate CSS files from Sass sources

Thanks for the review. Landed on master:
https://gitlab.gnome.org/GNOME/gnome-shell/commit/c62e7a6a
Comment 23 Michael Catanzaro 2018-02-12 22:50:24 UTC
Hi, please remember that a baby release team dies each time you add dependencies without updating gnome-build-meta! I've reported https://gitlab.com/BuildStream/debootstrap-ostree/merge_requests/3.
Comment 24 Mario Sánchez Prada 2018-02-13 11:00:42 UTC
Sorry about that Michael, and thanks for taking care of fixing my mess!