Add binary version configuration #42

dorxy · 2023-12-09T20:24:16Z

Resolves #3

dorxy · 2023-12-09T20:25:18Z

@weaverryan if this is the way to go I'll do the same for the twig bundle! 😄

WebMamba

Hey @dorxy thanks you for your time, and your interest in this 🧡

WebMamba · 2023-12-10T14:49:36Z

config/services.php

@@ -19,6 +19,7 @@
                abstract_arg('path to css directory'),
                param('kernel.project_dir'),
                abstract_arg('path to binary'),
+                abstract_arg('path to binary version'),


I think this is not the path to binary version but simply the binary version ?

WebMamba · 2023-12-10T14:52:00Z

doc/index.rst

+    symfonycasts_sass:
+        version: 1.69.0
+
+When you change this version, it will not be upgraded automatically. Remove the `var/dart-sass` directory first to rebuild with the configured version.


What do you think about using the binary version in the directory name containing the binary, so when we change the version it know that it should download the new version?

That would be sweet! It would solve WTF moments when it's cached with older version 👍

sounds like a great solution, I'll implement and document it!

WebMamba · 2023-12-10T14:53:01Z

src/DependencyInjection/SymfonycastsSassExtension.php

@@ -31,7 +31,8 @@ public function load(array $configs, ContainerBuilder $container): void
            ->replaceArgument(0, $config['root_sass'])
            ->replaceArgument(1, '%kernel.project_dir%/var/sass')
            ->replaceArgument(3, $config['binary'])
-            ->replaceArgument(4, $config['embed_sourcemap'])
+            ->replaceArgument(4, $config['version'])


I think we should keep the name binary_version. This is the naming we use in the code and it make think more clear.

src/DependencyInjection/SymfonycastsSassExtension.php

src/SassBinary.php

WebMamba · 2023-12-10T15:04:43Z

tests/VersionTest.php

@@ -0,0 +1,40 @@
+<?php


I think this test should be on the FunctionalTest class

bocharsky-bw

Hm, this is cool to be able to manually set any version you want for your project... but I think I would love to always download the latest version of Sass by default. Does it make sense for others too? IMO I would like to constraint (rollback) to the (old) legacy version ONLY if the latest version does not work for me for some reason or has a bug. I wonder if we can somehow download the latest version from GitHub without having to manually specify the exact version number in config?

UPD: Seems like there's a way: SymfonyCasts/tailwind-bundle#34

Any thoughts about it?

bocharsky-bw · 2023-12-14T15:33:03Z

src/SassBuilder.php

@@ -25,6 +25,7 @@ public function __construct(
        private readonly string $cssPath,
        private readonly string $projectRootDir,
        private readonly ?string $binaryPath,
+        private readonly ?string $binaryVersion,


hm, this will lead to BC breaks. What about to move this to the end instead?

src/SassBinary.php

bocharsky-bw · 2023-12-14T15:41:56Z

src/DependencyInjection/SymfonycastsSassExtension.php

@@ -31,7 +31,8 @@ public function load(array $configs, ContainerBuilder $container): void
            ->replaceArgument(0, $config['root_sass'])
            ->replaceArgument(1, '%kernel.project_dir%/var/sass')
            ->replaceArgument(3, $config['binary'])
-            ->replaceArgument(4, $config['embed_sourcemap'])
+            ->replaceArgument(4, $config['version'])


doc/index.rst

bocharsky-bw · 2023-12-14T15:47:48Z

doc/index.rst

+    symfonycasts_sass:
+        version: 1.69.0
+
+When you change this version, it will not be upgraded automatically. Remove the `var/dart-sass` directory first to rebuild with the configured version.


That would be sweet! It would solve WTF moments when it's cached with older version 👍

src/SassBinary.php

dorxy · 2023-12-15T08:53:29Z

I apologize for the late response, apparently GitHub either didn't notify me of all the conversations or my spam filter got them. I'll fix all the comments!

dorxy · 2023-12-15T10:05:02Z

@bocharsky-bw I like the idea of downloading the latest version, we would however not be able to show the default config for it with bin/console debug:config SymfonycastsSassBundle, and I'd be interested in your thoughts on automatically upgrading any version with for example a new deploy, the developer having no control over it.

In case there is no explicit version configured in the binary_version entry, we could do something like:

If there is no version downloaded yet download the latest version and set it in your projects config
Introduce a sass:upgrade command which downloads the latest available binary and updates the projects config

It would even be possible to explicitly enable the 'download the latest version I don't care if it breaks' by allowing a 'latest' string in the binary_version entry, which always checks for a newer version whenever your building. We could take a minimum of 1 day checking fmtime of the binary to not do version check requests for every time you build.

bocharsky-bw · 2023-12-15T15:31:29Z

@dorxy OK, let's see the big picture here: I think we definitely want to get the latest version of Sass binary by default here. But we also want to be able to specify which version you want to use via a config file (it may be useful for legacy purposes, e.g. when the latest version causes problems and you want to rollback to a specific version). If the user specified a version in the config - we will use it. If not - we will use the latest available. To solve the problem with upgrading - we need to use the version in the path to the Sass binary file, e.g. /path/to/sass/binary/1.63.6/ - this will solve the problem with the upgrade. Whenever a new Sass release comes up - the bundle will automatically fetch it. I think this is what is almost done for the Tailwind bundle here: SymfonyCasts/tailwind-bundle#38 - the path that contains the binary version will solve the problem with upgrading, i.e. you will not need a specific command for it, it will work behind the scene.

Yeah, debug:config will not show the version, but that's fine I think, if you care about the specific version - just specify it in the config and the bundle will use it always. If you don't care - you should count on the latest version always.

# Conflicts: # config/services.php # doc/index.rst # src/DependencyInjection/SymfonycastsSassExtension.php # src/SassBinary.php # src/SassBuilder.php # tests/SassBuilderTest.php

src/DependencyInjection/SymfonycastsSassExtension.php

tests/FunctionalTest.php

bocharsky-bw

I think failed tests is the only blocker that stops us from merging it

bocharsky-bw · 2024-01-15T12:12:05Z

Only Windows-related tests left... any ideas how to fix them?

c33s · 2024-02-20T01:27:07Z

the testlogfiles have expired, can someone retrigger the tests?

…-3-binary-version

bocharsky-bw · 2024-02-20T11:09:56Z

I merged the latest changes from main and retriggered the build - as you can see test failures are relevant.

@dorxy do you have some time to finish it? It would love to merge it when tests pass

c33s

@bocharsky-bw thanks for the retrigger, i hoped to see more in the ci but i had to run the code locally to debug it. i hope my info helps.

c33s · 2024-02-20T16:16:31Z

src/SassBinary.php

@@ -86,27 +94,30 @@ public function downloadExecutable(): void

        if ($isZip) {


public function testVersionDownloaded(): void { $testedVersion = '1.69.5'; // This should differ from the latest version which downloaded by default $binary = new SassBinary(binaryDownloadDir: __DIR__.'/fixtures/var/version', binaryVersion: $testedVersion); $binary->downloadExecutable(); exit; $this->assertDirectoryExists(__DIR__.'/fixtures/var/version/dart-sass/1.69.5'); $sassVersionProcess = new Process([__DIR__.'/fixtures/var/version/dart-sass/1.69.5/sass', '--version']); $sassVersionProcess->run(); $this->assertSame(trim($sassVersionProcess->getOutput(), \PHP_EOL), $testedVersion); }

the main problem of the failing test testVersionDownloaded (and i assume also the cause for the other failing tests) is this condition. php inspections (ea extended) the awesome plugin of phpstorm warns here that the else block can be extracted as the if block does a return. so for linux we download a tar/tar.gz file and the windows users are "stuck" in the if block and get returned. so there is no renaming to a path with the version inside.

also the zip file has a subdirectory called src this leads to a non-accessible binary

i also tried to run the tests on my alpine system, it is quite strange as they also fail. on linux my exit is ignored and all tests are running (i just wanted to create a screenshot of the resulting directory structure after download).

not sure if the code is not that stable or my linux bugging.

i am also confused why this wasn't a problem before,

c33s · 2024-02-20T16:16:53Z

src/SassBinary.php


            // delete the .tar (the .tar.gz is deleted below)
            unlink(substr($targetPath, 0, -3));
        }

        unlink($targetPath);

-        $binaryPath = $this->getDefaultBinaryPath();
+        // Rename the extracted directory to its version
+        rename($this->binaryDownloadDir.'/dart-sass/dart-sass', $this->binaryDownloadDir.'/dart-sass/'.$this->getVersion());


this is never reached for windows (zip download)

c33s · 2024-02-20T16:24:16Z

src/SassBinary.php

+
+            return $response->toArray()['tag_name'] ?? throw new \Exception('Cannot get the latest version name from response JSON.');
+        } catch (\Throwable $e) {
+            throw new \Exception('Cannot determine latest Dart Sass CLI binary version. Please specify a version in the configuration.', previous: $e);


we have no error handling for rate limits in here, in my tests i get a 403 from github and the resulting error message can be confusing without reading to the caused by part

Symfonycasts\SassBundle\Tests\SassBuilderTest::testSassOptionQuiet Exception: Cannot determine latest Dart Sass CLI binary version. Please specify a version in the configuration. /sass-bundle/src/SassBinary.php:191 /sass-bundle/src/SassBinary.php:181 /sass-bundle/src/SassBinary.php:38 /sass-bundle/src/SassBuilder.php:89 /sass-bundle/tests/SassBuilderTest.php:165 Caused by Symfony\Component\HttpClient\Exception\ClientException: HTTP/2 403 returned for "https://api.github.com/repos/sass/dart-sass/releases/latest".

bocharsky-bw · 2024-02-21T11:01:14Z

Hey @c33s , thank you a lot for such details here - pretty useful debugging!

Since @dorxy is probably busy and isn't active on it lately, do you want to fork this PR (basically base yours on top of https://github.com/dorxy/sass-bundle/tree/issue-3-binary-version ) and help to finish? Otherwise, I can take a look at it closer to the end of the week, I really would like to see this feature released.

dorxy · 2024-02-21T11:03:59Z

I'm sorry for not getting back to you guys sooner, @c33s thanks a lot for trying to dive into the issue!
I'd copy pasted from the previous solution before my implementation on downloading the executables, so I'm not sure why it's not passing the tests anymore. Perhaps we can compare it with the tailwind version to see how the downloads are happening there (and passing tests)?

c33s · 2024-02-24T23:34:10Z

@bocharsky-bw @dorxy just tell me if i can help.
i am currently quite busy but as soon as my load lightens i can do a fork if @dorxy gets stuck. but i can do smaller tests/debuggings on windows if you need

Add binary version configuration

5621b5d

WebMamba reviewed Dec 10, 2023

View reviewed changes

bocharsky-bw requested changes Dec 14, 2023

View reviewed changes

dorxy added 2 commits December 15, 2023 10:52

Improve naming for binary version configuration

771b3e3

Save dart binary in version directory, move version test

d6a74c5

dorxy added 2 commits January 6, 2024 14:43

Download latest Dart sass binary unless explicitly configured

8403495

Merge branch 'main' into issue-3-binary-version

0e958fc

# Conflicts: # config/services.php # doc/index.rst # src/DependencyInjection/SymfonycastsSassExtension.php # src/SassBinary.php # src/SassBuilder.php # tests/SassBuilderTest.php

bocharsky-bw reviewed Jan 9, 2024

View reviewed changes

Update configuration argument order

cda8510

bocharsky-bw reviewed Jan 12, 2024

View reviewed changes

Remove unneeded option argument in tests

5b522c0

bocharsky-bw added 2 commits February 20, 2024 12:02

Merge branch 'main' of github.com:SymfonyCasts/sass-bundle into issue…

f92e9d8

…-3-binary-version

Fix bad merging - remove duplicated method

f9f8acc

c33s suggested changes Feb 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add binary version configuration #42

Add binary version configuration #42

dorxy commented Dec 9, 2023

dorxy commented Dec 9, 2023

WebMamba left a comment

WebMamba Dec 10, 2023

WebMamba Dec 10, 2023

bocharsky-bw Dec 14, 2023

dorxy Dec 15, 2023

WebMamba Dec 10, 2023

bocharsky-bw Dec 14, 2023

WebMamba Dec 10, 2023

bocharsky-bw left a comment •

edited

Loading

bocharsky-bw Dec 14, 2023

bocharsky-bw Dec 14, 2023

bocharsky-bw Dec 14, 2023

dorxy commented Dec 15, 2023

dorxy commented Dec 15, 2023 •

edited

Loading

bocharsky-bw commented Dec 15, 2023

bocharsky-bw left a comment

bocharsky-bw commented Jan 15, 2024

c33s commented Feb 20, 2024

bocharsky-bw commented Feb 20, 2024 •

edited

Loading

c33s left a comment

c33s Feb 20, 2024

c33s Feb 20, 2024

c33s Feb 20, 2024

bocharsky-bw commented Feb 21, 2024

dorxy commented Feb 21, 2024

c33s commented Feb 24, 2024

		@@ -86,27 +94,30 @@ public function downloadExecutable(): void

		if ($isZip) {

Add binary version configuration #42

Are you sure you want to change the base?

Add binary version configuration #42

Conversation

dorxy commented Dec 9, 2023

dorxy commented Dec 9, 2023

WebMamba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bocharsky-bw left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dorxy commented Dec 15, 2023

dorxy commented Dec 15, 2023 • edited Loading

bocharsky-bw commented Dec 15, 2023

bocharsky-bw left a comment

Choose a reason for hiding this comment

bocharsky-bw commented Jan 15, 2024

c33s commented Feb 20, 2024

bocharsky-bw commented Feb 20, 2024 • edited Loading

c33s left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bocharsky-bw commented Feb 21, 2024

dorxy commented Feb 21, 2024

c33s commented Feb 24, 2024

bocharsky-bw left a comment •

edited

Loading

dorxy commented Dec 15, 2023 •

edited

Loading

bocharsky-bw commented Feb 20, 2024 •

edited

Loading