Skip to content

Add PCRE2 compatibility patch#60

Closed
jaimergp wants to merge 7 commits intoconda-forge:mainfrom
jaimergp:pcre2-compat
Closed

Add PCRE2 compatibility patch#60
jaimergp wants to merge 7 commits intoconda-forge:mainfrom
jaimergp:pcre2-compat

Conversation

@jaimergp
Copy link
Copy Markdown
Member

@jaimergp jaimergp commented Oct 7, 2022

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

Adds patch from openSUSE/libsolv#506.

This enables supports for properly merged glob strings in conda-libmamba-solver, as implemented in conda/conda#11612

Please review the patch, this is a bit beyond my comfort zone, both CMake and C 😬

@conda-forge-linter
Copy link
Copy Markdown

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@jaimergp
Copy link
Copy Markdown
Member Author

jaimergp commented Oct 7, 2022

@conda-forge-admin, please rerender

@wolfv
Copy link
Copy Markdown
Member

wolfv commented Oct 7, 2022

nice!

There are two additional considerations -- we should figure out if we can have a pcre2-static on conda-forge (for micromamba) and also add a recipe to boa-forge :)

@jaimergp
Copy link
Copy Markdown
Member Author

PCRE2 PR submitted here: conda-forge/pcre2-feedstock#31

@jezdez
Copy link
Copy Markdown
Member

jezdez commented Oct 11, 2022

For defaults the patch is in AnacondaRecipes#2

@jaimergp
Copy link
Copy Markdown
Member Author

@wolfv, PCRE2 10.40 is now available with a static lib too. Let me know if you need something else!

@wolfv
Copy link
Copy Markdown
Member

wolfv commented Oct 14, 2022

nice!

Comment thread recipe/meta.yaml
@jaimergp
Copy link
Copy Markdown
Member Author

I think we need a pcre2 migrator to push 10.37 to 10.40 if you want the static libs now.

@jaimergp
Copy link
Copy Markdown
Member Author

Also note this warning:

The install/build script(s) for libsolv deleted the following files (from dependencies) from the prefix:
['lib/libbz2.a', 'lib/libz.a']
This will cause the post-link checks to mis-report. Please try not to delete and files (DSOs in particular) from the prefix
WARNING:conda_build.build:The install/build script(s) for libsolv deleted the following files (from dependencies) from the prefix:
['lib/libbz2.a', 'lib/libz.a']

So it looks like the build scripts do not want static libs at all, and hence we shouldn't depend on pcre2-static? Wouldn't the libsolv compilation create the static lib on its own from the shared libs anyway?

Comment thread recipe/meta.yaml Outdated
@jaimergp
Copy link
Copy Markdown
Member Author

jaimergp commented Nov 1, 2022

@wolfv ping :D

@wolfv
Copy link
Copy Markdown
Member

wolfv commented Nov 1, 2022

I am about to release mamba / libmamba / micromamba 1.0 -- if it's OK let's wait after the release? I am also fixing some libarchive / curl issues in feedstocks specific to micromamba right now...

I really want to get this in, btw. sorry if it appears as if I was stalling this.

@jaimergp
Copy link
Copy Markdown
Member Author

jaimergp commented Nov 1, 2022

1.0 👀 That's exciting!!

No worries, I had forgotten myself 😬 I was just going through my GH notifications and found it again, fortunately. It's not super urgent, but it's blocking conda/conda#11612

@jaimergp
Copy link
Copy Markdown
Member Author

jaimergp commented Dec 6, 2022

@wolfv gentle reminder :D

@jaimergp jaimergp closed this Dec 6, 2022
@jaimergp jaimergp reopened this Dec 6, 2022
@jaimergp
Copy link
Copy Markdown
Member Author

jaimergp commented Dec 6, 2022

@conda-forge-admin, please rerender

Copy link
Copy Markdown

@varlackc varlackc left a comment

Choose a reason for hiding this comment

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

There are currently some merge conflicts on the branch.
We might need to resolve the conflicts before approving.

In addition it might be a good idea to also add the doc_url.

@jaimergp
Copy link
Copy Markdown
Member Author

@conda-forge-admin, please rerender

@conda-forge-webservices
Copy link
Copy Markdown
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I was trying to look for recipes to lint for you, but it appears we have a merge conflict.
Please try to merge or rebase with the base branch to resolve this conflict.

Please ping the 'conda-forge/core' team (using the @ notation in a comment) if you believe this is a bug.

@AntoinePrv
Copy link
Copy Markdown
Contributor

@jaimergp is this for libmamba?

@jaimergp
Copy link
Copy Markdown
Member Author

This is required for compatibility with conda/conda#11612, which requires advanced regexes internally. But if libmamba is going to be parsing things outside libsolv now maybe we don't need it anymore?

@AntoinePrv
Copy link
Copy Markdown
Contributor

That would be our plan, but it is still unclear how we can plug a matchspec callback into libsolv.
But isn't matching not trusted regex from repodata.json a DOS security risk?

@jaimergp
Copy link
Copy Markdown
Member Author

We can still sanitize user input and avoid the DOS (which is already a problem today in current conda, given it allows pkg[build=/^some[-]regex$/] specs. Those advanced regexes are generated internally to accommodate the merge of several *build_string* globs à la *_gpu* or _*gpu_cuda11 (these two are compatible but conda would reject them now).

@AntoinePrv
Copy link
Copy Markdown
Contributor

I was thinking of only allowing them in user input, not package input.
The globs are different from the regex aren't they? They are already implemented libsolv AFAIK.

@jaimergp
Copy link
Copy Markdown
Member Author

The globs are converted internally to regexes after they pass the merge step (which in current conda is only checking whether they are the same string or not). What the PR I linked above proposed is using lookahead regexes to handle the merge (instead of strict equals). The discussion there contains all the details you need!

@AntoinePrv
Copy link
Copy Markdown
Contributor

On Mamba the globs are not converted to regex, and I think also not on libsolv.

@jaimergp
Copy link
Copy Markdown
Member Author

With conda-libmamba-solver, we take the MatchSpec objects as generated by conda. If that PR was to be merged, then we could receive MatchSpec objects with a build string that has a lookahead regex, and then libsolv would crash because that kind of regex is not supported by the current regex engine. It does accept simpler regexes, just not lookaheads and similar.

@AntoinePrv
Copy link
Copy Markdown
Contributor

Closing as stale and conflicted. Feel free to reopen.

@AntoinePrv AntoinePrv closed this Aug 4, 2025
@jaimergp
Copy link
Copy Markdown
Member Author

jaimergp commented Aug 4, 2025

I can confirm we don't need this anymore because of the new mamba matchspec parser.

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.

6 participants