Piggyback migrator for boost unification #1668
Conversation
h-vetinari
force-pushed
the
boost
branch
2 times, most recently
from
f62bc72 to
6655299
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1668 +/- ##
==========================================
+ Coverage 68.18% 68.65% +0.46%
==========================================
Files 91 93 +2
Lines 8701 8789 +88
==========================================
+ Hits 5933 6034 +101
+ Misses 2768 2755 -13
☔ View full report in Codecov by Sentry. |
|
@beckermr, any thoughts here? In case this needs a large overhaul, I'd like to tackle it. But if it look +/- alright, I'm happy to let it lie until we finish conda-forge/boost-feedstock#164 |
|
This PR is pretty hefty so I have not had a chance to look in detail. The test suite looks amazing and so I'm very happy to trust you on the details. Let's let this one wait until things are ready to move ahead. |
It might be scary-looking, but in terms of LoC, it's <7% implementation, <2% tests & >90% test cases. That's why I split it into separate commits. ;-)
Happy to hear it, thanks for taking a look!
SGTM |
h-vetinari
changed the title
|
This has now been updated to the new naming adopted in conda-forge/boost-feedstock#164 |
I don't mind pinging you when I'm ready, but if you're happy to share commit access, sure.
I asked for any last comments on conda-forge/boost-feedstock#164, but clearly that PR needs to be merged first. However, w.r.t. the migration, we're now exactly in the situation we wanted to avoid - coinciding with the 3.12 migration. I guess we could still squeak in boost before 3.12 kicks off in earnest? Talking about what we need for this to be merged; I also think we need to merge a corresponding pinning PR (the base of the piggyback)? At least for #1602 (which I was basing myself off of) there was a special migrator; I can prepare that once conda-forge/boost-feedstock#164 is merged... |
|
Ah right. I'll add you here in a bit. You should go ahead with what you need. |
|
Still trying to follow #1602 as closely as I could1, I've now paired this with conda-forge/conda-forge-pinning-feedstock#4961. Footnotes
|
|
Alright, here goes nothing! 😅 |
This is
a draftthe implenentation for conda-forge/boost-feedstock#164TL;DR
The main rule is:
boost-cppis only a host dep, replace it withlibboost-headers(that doesn't have nor need a run-export)boost-cppis also a run dep, remove it there and turn the host dep intolibboost-devel(which has a run-export)Additionally,
boostis renamed tolibboost-python-devel.Implementation
I tried to base myself on #1602, but found that I needed some more control, especially for multi-output recipes. Since everything is already parsed and re-parsed many times, I went with a dead-simple solution that avoids having to identify exactly where in the meta.yaml a given requirement comes from, but have a broad section (based on the outputs that we know) in which to do replacements. It's possible that this already exists (or should exist?) in the utils here...?
The implementation is already pretty involved due to the fact that while operating on the unstructured strings, it's hard to tell what's a host or a run dependence, and we have to fall back to some assumptions involving how many times we want to replace or discard something. This gets more complicated by the occurrence of our key strings in build dependencies or comments. Of course, there are always things that are going to break the logic here, but I think it should hopefully handle 90+% of feedstocks correctly, and I think there are diminishing returns for upping the complexity of the implementation further.
Tests
I added a bunch of tests for feedstocks that showed up when searching for boost-cpp or boost, and I'm quite happy that even some of the trickier setups (pins, selectors, etc.) are handled correctly.
These were hard to write (because the machinery in play is very hard to understand IMO), so happy to improve. I ended up replacing the source sections of all sample recipes with scipy, because the version migrator (that's necessary to test the piggybacking, because we cannot test an actual pin migration AFAICT) would otherwise choke for reasons that aren't clear to me. Aside from that (and light editing that the migrator insists on, like removing quotes or changing selector spacing), the recipes are as they are on their current feedstocks.
I kept the tests in separate commits, because they're hard to read in the GH UI. For the benefit of the reader, here is the diff between the before/after file:
Single output; only host-dependence on boost-cpp
git diff HEAD:tests/test_yaml/libboost_gudhi_before_meta.yaml HEAD:tests/test_yaml/libboost_gudhi_after_meta.yamlSingle output; host- & run-dependence on boost-cpp
git diff HEAD:tests/test_yaml/libboost_carve_before_meta.yaml HEAD:tests/test_yaml/libboost_carve_after_meta.yamlMultiple outputs; tricky because appearance in comment messes up naïve counting
git diff HEAD:tests/test_yaml/libboost_fenics_before_meta.yaml HEAD:tests/test_yaml/libboost_fenics_after_meta.yamlI consider the fact that the patch name gets spuriously renamed to be acceptable.
Multiple outputs; tricky due to selectors,
{{ }}pins & pin_compatiblegit diff HEAD:tests/test_yaml/libboost_poppler_before_meta.yaml HEAD:tests/test_yaml/libboost_poppler_after_meta.yamlMultiple outputs; tricky due to duplicate host entries & version pins
git diff HEAD:tests/test_yaml/libboost_scipopt_before_meta.yaml HEAD:tests/test_yaml/libboost_scipopt_after_meta.yamlPython output (
boost -> libboost-python-devel): simple casegit diff HEAD:tests/test_yaml/libboost_rdkit_before_meta.yaml HEAD:tests/test_yaml/libboost_rdkit_after_meta.yamlPython output (
boost -> libboost-python-devel): interaction withboost-cppgit diff HEAD:tests/test_yaml/libboost_cctx_before_meta.yaml HEAD:tests/test_yaml/libboost_cctx_after_meta.yaml