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

Add support for build scss files on hatch build #1363

Merged
merged 16 commits into from
May 22, 2024

Conversation

oscarmcm
Copy link
Member

Initial work for:


SCSS changes:

diff --git a/src/wiki/static/wiki/bootstrap/scss/wiki/wiki.scss b/src/wiki/static/wiki/bootstrap/scss/wiki/wiki.scss
index b12569d0..6d86709c 100644
--- a/src/wiki/static/wiki/bootstrap/scss/wiki/wiki.scss
+++ b/src/wiki/static/wiki/bootstrap/scss/wiki/wiki.scss
@@ -11,21 +11,23 @@ $danger:              #721817;
 {
     background: $info;
 }
 
 @mixin wiki-page-header{
     @extend .pb-2;
     @extend .mt-4;
     @extend .mb-2;
     @extend .border-bottom;
 }
-
+.some-very-long-class {
+  color: yellow;
+}
 .wiki-article .thumbnail
 {
     clear: both;
     margin-bottom: 15px;
     margin-left: 10px;
     margin-right: 10px;
     text-align: center;
 }

Output:
image

@oscarmcm oscarmcm added the enhancement A feature request: Will be implemented if someone steps up! label May 21, 2024
@oscarmcm oscarmcm added this to the 0.12 milestone May 21, 2024
@oscarmcm oscarmcm requested a review from benjaoming May 21, 2024 18:41
@oscarmcm oscarmcm self-assigned this May 21, 2024
@oscarmcm
Copy link
Member Author

I moved away from pure SCSS implementation and used PySCSS since those provide pre-built wheels for Linux and maxOS

Copy link

codecov bot commented May 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.75%. Comparing base (050f124) to head (176f6db).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1363   +/-   ##
=======================================
  Coverage   79.75%   79.75%           
=======================================
  Files         114      114           
  Lines        4737     4737           
  Branches      637      637           
=======================================
  Hits         3778     3778           
  Misses        748      748           
  Partials      211      211           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@oscarmcm
Copy link
Member Author

@benjaoming I'll need help figuring out how to add libsassc inside of the read the docs CI

@benjaoming
Copy link
Member

Awesome work!

This will make it much easier to build releases without maintaining the CSS artifact in git.

Maybe we can argue this way? "libsass" and "pysass" are build dependencies. So we need to define the build dependencies explicitly (not sure what is the "hatch way" of doing that)

For now, I've added it as a docs dependency... but maybe there's a better way?

@benjaoming
Copy link
Member

Ah, this reminds me of when setup.py had a hack to specify that it needed some requirements to be installed BEFORE installing with setup_requires.

@benjaoming
Copy link
Member

I think the current error is something about the build environment being isolated from the environment that the documentation dependencies are installed into. So I guess that whole approach won't work 😖

@oscarmcm
Copy link
Member Author

Yeah, that's the thing!

Our approach works for CircleCI since we can have packages installed on each matrix environment. And since this is not allowed I dont think it will work with the RTD CI environment.

I dont think that marking this as a build dependency is a good idea, I really like the approach to have concerns separate and dont block Python development, just because a contributor doesn't have properly configured a SCSS installation.

Do you know if there's a way to check if the RDT CI has an environment variable that we can read and conditionally run the command?

@benjaoming
Copy link
Member

Yes, you can define environment variables on RTD 👍

@benjaoming
Copy link
Member

I really like the approach to have concerns separate and dont block Python development, just because a contributor doesn't have properly configured a SCSS installation.

Agreed!

@oscarmcm
Copy link
Member Author

@benjaoming this should be good to go :)

@@ -1,5 +1,5 @@
[build-system]
requires = ["hatchling"]
requires = ["hatchling", "hatch-build-scripts"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! So I guess that it's not possible to list libsass here?

I've clicked "watching" on the hatch-build-scripts repo since it seems like it's a small project: https://github.com/rmorshea/hatch-build-scripts

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good observation!!

Based on the PEP documentation, we can place it there (https://peps.python.org/pep-0518/) I could explore that just in case, the only thing I'm not OK is that using this solution will make the lib sass required for everything, since for each environment this is build on dev mode.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a bad idea. I don't have any strong opinions on this one, if the libsass dependency isn't really problematic and pysassc is fast to run, then it's okay for me. But we can easily just wait and see if the new setup causes any problems, it also seems fine as it is 👍

[[tool.hatch.build.hooks.build-scripts.scripts]]
out_dir = "src/wiki/static/wiki/bootstrap/css/"
commands = [
"[[ -z $READTHEDOCS_VIRTUALENV_PATH ]] && pysassc --style compressed src/wiki/static/wiki/bootstrap/scss/wiki/wiki-bootstrap.scss src/wiki/static/wiki/bootstrap/css/wiki-bootstrap.min.css || true",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great solution 👍

@benjaoming benjaoming merged commit 3138ec9 into main May 22, 2024
17 checks passed
@benjaoming benjaoming deleted the feature/build-scss-on-release branch May 22, 2024 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A feature request: Will be implemented if someone steps up!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants