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

libpsl: add v0.21.2, bump deps, simplify patching #22029

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

valgur
Copy link
Contributor

@valgur valgur commented Dec 28, 2023

.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@eli-schwartz
Copy link

How does this "simplify patching"? Patch files can be cleanly verified to still apply, doing the equivalent of a sed will appear to work but potentially do nothing or potentially break things without providing any feedback that it didn't do what's expected.

@valgur
Copy link
Contributor Author

valgur commented Dec 31, 2023

@eli-schwartz It does not, from the perspective of the package consumer, at least. But it achieves the exact same result with a much lower maintenance burden. The changes here are relatively trivial and easy to re-apply, but re-applying such patches really adds up across hundreds of packages, when trying to keep most of them up to date. "Improve maintainability" would have been a better choice of words.

doing the equivalent of a sed will appear to work but potentially do nothing or potentially break things without providing any feedback that it didn't do what's expected

It's a valid concern, but the replace_in_file method accounts for this and defaults to strict=True, which results in an error being raised in case of the replacement not being applied.

@eli-schwartz
Copy link

That's the "or potentially break things" case I mentioned. Seds have a distressing tendency to break due to upstream changes that move code around so the sed still does something, just not, well, the thing you originally intended it to do.

@eli-schwartz
Copy link

Anyways, upstream has already changed this to support -Dtests=false which will disable this without any form of replacements or patching. Which is proof that you should NOT use a sed. The modifications are only needed for one version, and the next release tag, a patch will stop applying (but a sed will still apply) and you want to be notified that the patch stops applying, because it is your notification that you should stop modifying the file and start passing a build option instead.

This is a small part of why I hate seds, whether they are implemented in python with strict parameters or not.

@valgur
Copy link
Contributor Author

valgur commented Dec 31, 2023

Anyways, upstream has already changed this to support -Dtests=false which will disable this without any form of replacements or patching. Which is proof that you should NOT use a sed.

It's still working as expected.

Copy link
Contributor

github-actions bot commented Feb 7, 2024

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 3 (e22407068cc5c7d0b0370089bd77d722b12c68ad):

  • libpsl/0.21.5:
    All packages built successfully! (All logs)

  • libpsl/0.21.1:
    All packages built successfully! (All logs)


Conan v2 pipeline ✔️

Note: Conan v2 builds are now mandatory. Please read our discussion about it.

All green in build 3 (e22407068cc5c7d0b0370089bd77d722b12c68ad):

  • libpsl/0.21.5:
    All packages built successfully! (All logs)

  • libpsl/0.21.1:
    All packages built successfully! (All logs)

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.

3 participants