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

[BINARY] : Bump binary to 1.69.7 #51

Merged
merged 8 commits into from
Jan 5, 2024

Conversation

LaurentSanson
Copy link
Contributor

@LaurentSanson LaurentSanson commented Dec 31, 2023

As it is mentioned in the issue 31, the sass binary has been updated and it now provides official releases for musl LibC and for Android.

Closes #31

@smnandre
Copy link
Contributor

The stop_on_error seems to be ignored :/ i’ll look tomorrow

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.

Great! This should finally close #31 🎉

Though I wonder if the bumped version is the reason why tests fail here

@andersonamuller
Copy link

To work with Alpine we would still need to detect that on the getBinaryName method to add the -musl

@bocharsky-bw
Copy link
Member

@andersonamuller would you like to open a PR for this change?

@andersonamuller
Copy link

Sorry @bocharsky-bw, I just wated to bring to attention, no time ATM for a PR. We are not even using the bundle yet as the Alpine was a requirement and we had to incorporate in our docker build the download of SASS binary.

@bocharsky-bw
Copy link
Member

Yeah sure, thanks for the head-ups though! If there's a volunteer who would like to help with necessary changes for Alipnie support in this bundle - feel free to take it

@LaurentSanson
Copy link
Contributor Author

@bocharsky-bw , I've added the -musl option to get the binary file, but we still got errors. If you have any idea why, please let me know

src/SassBinary.php Outdated Show resolved Hide resolved
@bocharsky-bw bocharsky-bw changed the title [BINARY] : Bump binary to 1.69.6 [BINARY] : Bump binary to 1.69.7 Jan 3, 2024
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.

The code looks good to me now 👍

Unfortunately, as you can see with my latest few commits - test failures are related here. Could you take a look?

@smnandre
Copy link
Contributor

smnandre commented Jan 3, 2024

The errors seems to be only related to stop_on_error option right ?

@bocharsky-bw
Copy link
Member

bocharsky-bw commented Jan 5, 2024

@smnandre yeah happens in testSassOptionStopOnError()

hm, the --stop-on-error arg is there, but seems like the new dart just ignores it?

@smnandre
Copy link
Contributor

smnandre commented Jan 5, 2024

I think it's related to recent changes in the async handling in sass. So we may remove temporary the test ? 👼

@bocharsky-bw
Copy link
Member

Ah, that sounds possible... OK, I temporarily marked it as skipped. If anybody has ideas on how to fix it (or test that in a better way) - please, open a PR.

Thank you guys!

@bocharsky-bw bocharsky-bw merged commit 50bc699 into SymfonyCasts:main Jan 5, 2024
11 checks passed
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.

[DOCS] add a note that this bundle does not work with alpine
4 participants