GNOME Bugzilla – Bug 792822
data: Automatically generate CSS files out of the SASS sources
Last modified: 2018-02-13 11:00:42 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
Mmh, it adds a build dependency on sassc, but since we switched away from ruby-sass, that may not be too bad.
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
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.
(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!
(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).
Review of attachment 367307 [details] [review]: As mentioned, this doesn't really fix CSS generation, but rather breaks it completely :-)
(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.
(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.
(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
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.
(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.
(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.
Created attachment 367320 [details] [review] data/theme/meson.build: Fix generation of CSS files out of SCSS ones
Ping? @Florian, let me know if you want me to move this one to gitlab and propose a MR there, thanks
(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
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
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
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
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!
Review of attachment 367991 [details] [review]: LGTM now.
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
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.
Sorry about that Michael, and thanks for taking care of fixing my mess!