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 binary version configuration #42

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

dorxy
Copy link

@dorxy dorxy commented Dec 9, 2023

Resolves #3

@dorxy
Copy link
Author

dorxy commented Dec 9, 2023

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

Copy link
Contributor

@WebMamba WebMamba left a comment

Choose a reason for hiding this comment

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

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

@@ -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'),
Copy link
Contributor

Choose a reason for hiding this comment

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

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

doc/index.rst Outdated
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

@@ -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'])
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Agree

src/SassBinary.php Show resolved Hide resolved
@@ -0,0 +1,40 @@
<?php
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this test should be on the FunctionalTest class

Copy link
Member

@bocharsky-bw bocharsky-bw left a comment

Choose a reason for hiding this comment

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

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?

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

Choose a reason for hiding this comment

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

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

src/SassBinary.php Show resolved Hide resolved
@@ -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'])
Copy link
Member

Choose a reason for hiding this comment

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

Agree

doc/index.rst Outdated Show resolved Hide resolved
doc/index.rst Outdated
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.
Copy link
Member

Choose a reason for hiding this comment

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

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

src/SassBinary.php Outdated Show resolved Hide resolved
src/SassBinary.php Outdated Show resolved Hide resolved
@dorxy
Copy link
Author

dorxy commented Dec 15, 2023

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
Copy link
Author

dorxy commented Dec 15, 2023

@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
Copy link
Member

@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 Outdated Show resolved Hide resolved
src/DependencyInjection/SymfonycastsSassExtension.php Outdated Show resolved Hide resolved
src/DependencyInjection/SymfonycastsSassExtension.php Outdated Show resolved Hide resolved
tests/FunctionalTest.php Outdated Show resolved Hide resolved
Copy link
Member

@bocharsky-bw bocharsky-bw left a comment

Choose a reason for hiding this comment

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

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

@bocharsky-bw
Copy link
Member

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

@c33s
Copy link

c33s commented Feb 20, 2024

the testlogfiles have expired, can someone retrigger the tests?

@bocharsky-bw
Copy link
Member

bocharsky-bw commented Feb 20, 2024

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

Copy link

@c33s c33s left a comment

Choose a reason for hiding this comment

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

@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.

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

if ($isZip) {
Copy link

Choose a reason for hiding this comment

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

    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

image

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,


// 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());
Copy link

Choose a reason for hiding this comment

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

this is never reached for windows (zip download)


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);
Copy link

Choose a reason for hiding this comment

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

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
Copy link
Member

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
Copy link
Author

dorxy commented Feb 21, 2024

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
Copy link

c33s commented Feb 24, 2024

@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

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

Successfully merging this pull request may close these issues.

Allow binary version to be configurable
5 participants