-
Notifications
You must be signed in to change notification settings - Fork 75
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
Piggyback migrator for stdlib-migration #2135
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2135 +/- ##
==========================================
+ Coverage 71.03% 71.46% +0.42%
==========================================
Files 97 99 +2
Lines 9674 9833 +159
==========================================
+ Hits 6872 7027 +155
- Misses 2802 2806 +4 ☔ View full report in Codecov by Sentry. |
The add-trailing-comma hook here produces a false positive: pat_compiler = re.compile(
r"(?P<indent>\s*)-\s*"
r"(?P<compiler>\{\{\s*compiler\([\"\'](c|cxx|fortran)[\"\']\)\s*\}\})"
- r"\s*(?P<selector>\#\s+\[[\w\s]+\])?"
+ r"\s*(?P<selector>\#\s+\[[\w\s]+\])?",
) This is a string that should not get a comma. Black handles trailing commas already, so I don't get why there's a separate hook. |
yeah that comma won't hurt but is odd. You can remove that hook in another PR if you'd like. |
Done: #2136 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice! I had a few questions in the comments. Also, we should add a test where the compiler has a selector. I didn't see that covered in the current tests though I may have missed it!
almost all outputs gain a host dependence on stdlib (including `apache-arrow-proc`, where we insert the host section due to the assumption that an output without a build section is relying on the global build requirements). Note that `libarrow-all` does not gain a stdlib-dependence, as it _has_ a non-empty build section. ``` diff --git a/tests/test_yaml/stdlib_arrow_before_meta.yaml b/tests/test_yaml/stdlib_arrow_after_meta.yaml index 85771bad..60b4550b 100644 --- a/tests/test_yaml/stdlib_arrow_before_meta.yaml +++ b/tests/test_yaml/stdlib_arrow_after_meta.yaml @@ -55,6 +55,7 @@ requirements: - autoconf # [linux] - make # [linux] host: + - {{ stdlib("c") }} # for required dependencies, see # https://github.com/apache/arrow/blob/apache-arrow-11.0.0/cpp/cmake_modules/ThirdpartyToolchain.cmake#L46-L75 - clangdev {{ llvm_version }} @@ -94,6 +95,8 @@ outputs: number: {{ proc_build_number }} string: {{ build_ext }} requirements: + host: + - {{ stdlib("c") }} run_constrained: # avoid installation with old naming of proc package - arrow-cpp-proc <0.0a0 @@ -185,6 +188,7 @@ outputs: - {{ compiler("cxx") }} - {{ compiler("cuda") }} # [cuda_compiler_version != "None"] host: + - {{ stdlib("c") }} - aws-crt-cpp - aws-sdk-cpp - brotli @@ -286,6 +290,7 @@ outputs: - {{ compiler("c") }} - {{ compiler("cxx") }} host: + - {{ stdlib("c") }} - {{ pin_subpackage("libarrow", exact=True) }} run: - {{ pin_subpackage("libarrow", exact=True) }} @@ -330,6 +335,7 @@ outputs: - {{ compiler("c") }} - {{ compiler("cxx") }} host: + - {{ stdlib("c") }} - {{ pin_subpackage("libarrow", exact=True) }} - {{ pin_subpackage("libarrow-acero", exact=True) }} - {{ pin_subpackage("libparquet", exact=True) }} @@ -381,6 +387,7 @@ outputs: - libgrpc # [build_platform != target_platform] - libprotobuf # [build_platform != target_platform] host: + - {{ stdlib("c") }} - {{ pin_subpackage("libarrow", exact=True) }} - libabseil - libgrpc @@ -435,6 +442,7 @@ outputs: - libgrpc # [build_platform != target_platform] - libprotobuf # [build_platform != target_platform] host: + - {{ stdlib("c") }} - {{ pin_subpackage("libarrow", exact=True) }} - {{ pin_subpackage("libarrow-flight", exact=True) }} - libprotobuf @@ -482,6 +490,7 @@ outputs: - {{ compiler("c") }} - {{ compiler("cxx") }} host: + - {{ stdlib("c") }} - {{ pin_subpackage("libarrow", max_pin="x") }} - libutf8proc # gandiva requires shared libllvm; needs to match version used at build time @@ -538,6 +547,7 @@ outputs: - cmake - ninja host: + - {{ stdlib("c") }} - {{ pin_subpackage("libarrow", exact=True) }} - {{ pin_subpackage("libarrow-acero", exact=True) }} - {{ pin_subpackage("libarrow-dataset", exact=True) }} @@ -588,6 +598,7 @@ outputs: - {{ compiler("c") }} - {{ compiler("cxx") }} host: + - {{ stdlib("c") }} - {{ pin_subpackage("libarrow", max_pin="x") }} - openssl - thrift-cpp @@ -647,6 +658,7 @@ outputs: - cmake - ninja host: + - {{ stdlib("c") }} - {{ pin_subpackage("libarrow-all", exact=True) }} - clangdev {{ llvm_version }} - llvmdev {{ llvm_version }} @@ -738,6 +750,7 @@ outputs: - cmake - ninja host: + - {{ stdlib("c") }} - {{ pin_subpackage("libarrow-all", exact=True) }} - {{ pin_subpackage('pyarrow', exact=True) }} - clangdev {{ llvm_version }} ```
``` diff --git a/tests/test_yaml/stdlib_polars_before_meta.yaml b/tests/test_yaml/stdlib_polars_after_meta.yaml index 4221271f..ae72755e 100644 --- a/tests/test_yaml/stdlib_polars_before_meta.yaml +++ b/tests/test_yaml/stdlib_polars_after_meta.yaml @@ -30,6 +30,7 @@ requirements: - make # [unix] - cargo-bundle-licenses host: + - {{ stdlib("c") }} # this is a hacky way to do cross-compilation from linux to windows - python # [not (build_platform == "linux-64" and target_platform == "win-64")] - pip # [not (build_platform == "linux-64" and target_platform == "win-64")] ```
due to limitations of the testing framework here, we still have different files because we need to go through a fake version migrator to test the piggyback.
Thanks for the quick review!
Indeed, there aren't that many variations tested yet (though the code is written at least) - I didn't have a feedstock to hand for that, but I can look for one. |
added characters until the following search was (essentially) empty: https://github.com/search?q=org%3Aconda-forge+%2F%23+%5C%5B.*%5B%5E%5Cw%5Cs%28%29%3C%3E%21%3D.%2C%5C-%5C%27%5C%22%5D.*%5C%5D%2F+path%3Arecipe%2Fmeta.yaml&type=code
@isuruf, would appreciate your input on this! 🙏 |
Yes we can write to the conda build config if needed. |
Cool, how would I do that? So far I've just been rewriting meta.yaml lines. 😅 |
The entire feedstock is pulled via git and the bot's cwd is in the feedstock. Thus you should be to detect if a cbc yaml is there and make the appropriate changes and/or create one as needed. I forget if you need to git add the file or not, but I suspect a Quick Look at the code will indicate what to do. |
Mainly I'm waiting for input from Isuru, as I'm implementing his idea, without having necessarily grasped all the details. Though the one thing you could help me with is the last point from the OP. I needed to teach the recipe parser how to handle the new jinja function (i.e. once recipes start using it), and so far I've only added it in |
Right so if the stdlib packages have run exports attached to them, then we need to make sure those get accounted for in make_graph.py. I'd be good to add a test with the new jinja2 to the check solvable repo as well (https://github.com/regro/conda-forge-feedstock-check-solvable). |
For windows we can just do
|
OK cool, thanks.
For now I just added this to
I think the next step for that would be to add |
yep LGTM on adding to the compiler stubs with exports list. Surely we'll find other issues later on, but I think we're good to go for now and not doing anything obviously dumb for the moment! |
Never mind, cannot ping across organisations... |
) | ||
pat_stdlib = re.compile(r".*\{\{\s*stdlib\([\"\']c[\"\']\)\s*\}\}.*") | ||
# no version other than 2.17 currently available (except 2.12 as default on linux-64) | ||
pat_sysroot_217 = re.compile(r"- sysroot_linux-64\s*=?=?2\.17") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pat_sysroot_217 = re.compile(r"- sysroot_linux-64\s*=?=?2\.17") | |
pat_sysroot = re.compile(r"- sysroot_linux-64\s*=?=?2\.(17|28)") |
We have 2.28 in the wild right now and formally people can build with it if they do not use cdts. Maybe we should account for it now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had checked at the time and there were no users of 2.28. Now that I've expanded the search a bit, there's only cudnn (which seems misspecified actually because it pins both 2.17 & 2.28 on aarch).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. As I meant to say totally optional.
needs_stdlib |= any(pat_compiler.search(line) for line in build_reqs) | ||
|
||
if not needs_stdlib: | ||
if any(pat_sysroot_217.search(line) for line in lines): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if any(pat_sysroot_217.search(line) for line in lines): | |
if any(pat_sysroot.search(line) for line in lines): |
|
||
# check if someone specified a newer sysroot in recipe already, | ||
# leave indicator to migrate() function that we need to write spec to CBC | ||
if any(pat_sysroot_217.search(line) for line in lines): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if any(pat_sysroot_217.search(line) for line in lines): | |
if any(pat_sysroot.search(line) for line in lines): |
lines = attrs["raw_meta_yaml"].splitlines() | ||
already_migrated = any(pat_stdlib.search(line) for line in lines) | ||
has_compiler = any(pat_compiler.search(line) for line in lines) | ||
has_sysroot = any(pat_sysroot_217.search(line) for line in lines) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
has_sysroot = any(pat_sysroot_217.search(line) for line in lines) | |
has_sysroot = any(pat_sysroot.search(line) for line in lines) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is ready to go. I made one set of comments but they are probably overkill.
Thanks a lot for the collaboration and help here! 🙏 |
First PRs are being opened, so far looking like it's working as intended 🤞 |
As the next big step for conda-forge/conda-forge.github.io#1844, write a piggyback-migrator for adding
{{ stdlib("c") }}
to all our compiled recipes. For now this is just aimed at one migration, with the aim to enable it for all migrations afterwards (see here).Done:
m2w64
-compilerssysroot_linux-64
with corresponding spec in CBCMACOSX_DEPLOYMENT_TARGET
in CBC, remove lines involving__osx
from meta.yamlTo do resp. to decide:
{{ compiler("fortran") }}
might use symbols from the C stdlib.{{ stdlib("c") }}
also on windows (even if empty)? If so, what willc_stdlib{,_version}
be? This influences if we would need to add/modify selectors, which I would very much like to avoid.{{ compiler(...) }}
for the output has a selector, copy that to{{ stdlib("c") }}
.attrs["meta_yaml"]["requirements"]
. Also, the current logic depends on finding the lines forbuild: / host: / run
, so this would need deeper changes._stub
handling are necessary in various files here, e.g.conda_forge_tick/{
migrators/arch.py
,
make_graph.py
,...}
We can easily add more tests if you point me at some feedstocks you want to check.
Reuses some useful functionality (for slicing outputs, resp. testing aproach) from #1668.