Skip to content
This repository has been archived by the owner on Mar 12, 2024. It is now read-only.

Add run_exports to boost-cpp #82

Closed

Conversation

jakirkham
Copy link
Member

@jakirkham jakirkham commented Sep 14, 2020

Pin all the way to the patch level as they include the patch version in the SONAME.

https://abi-laboratory.pro/index.php?view=timeline&l=boost

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.

Pin all the way to the patch level as they include the patch version in
the SONAME.

https://abi-laboratory.pro/index.php?view=timeline&l=boost
Rebuild now that `run_exports` is used.
@conda-forge-linter
Copy link

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.

@jakirkham
Copy link
Member Author

cc @conda-forge/core (for awareness)
cc @kkraus14

@beckermr
Copy link
Member

how did we not have this before?

Also, we should eventually remove the pin_run_as_build in the pinnings file once we have enough versions of this done with the constraint.

@kkraus14
Copy link

Just a note, a good portion of boost-cpp is header only that is not needed at runtime, certain modules like boost::filesystem are not which means a binary component is needed at runtime. I'm not sure what the typical path for conda-forge is in these situations as to whether the run export should be opt-in or opt-out.

@scopatz
Copy link
Member

scopatz commented Sep 14, 2020

@kkraus14 - in that case, there is a mechanism to avoid run-exports in the deps that need/want to

@jakirkham
Copy link
Member Author

It's a great question, Matt. Was also surprised. In any event, this should fix it 🙂

Yeah there's ignore_run_exports for skipping the pinning downstream.

@beckermr
Copy link
Member

Right. Downstreams can skip things. We could also further subdivide the boost package, making the header only parts a separate output and dep of boost-cpp.

@beckermr
Copy link
Member

Also, we should eventually remove the pin_run_as_build in the pinnings file once we have enough versions of this done with the constraint.

We could ship a repodata patch if we wanted to remove that now. I am agnostic on that.

Copy link
Member

@isuruf isuruf left a comment

Choose a reason for hiding this comment

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

We had a big discussion of not using run_exports in boost-cpp. I can't find the discussion right now.

Until all the recipes in conda-forge which use headers only are fixed with ignore_run_exports, -1 from me. If you can do the leg work to fix all the recipes before merging this, I'm okay with this.

@beckermr
Copy link
Member

We had a big discussion of not using run_exports in boost-cpp. I can't find the discussion right now.

Ahh ok. I didn't know about that until now. If you can find the text of it, I'd love to read to catch up.

In the meantime, maybe @jakirkham we should comment out the run export in the recipe and leave a comment on the underlying issues and steps needed to resolve them? This way newcomers don't have to backtrack and ultimately rely on the memories of ppl watching PRs to get it right.

@jakirkham
Copy link
Member Author

I'd rather just leave the PR open instead of merging with a comment. It didn't take us long to find the issue. Plus this makes it clear that this work still needs to be done. If there's relevant conversation, we can track it here as well.

@h-vetinari
Copy link
Member

h-vetinari commented Nov 2, 2022

Until all the recipes in conda-forge which use headers only are fixed with ignore_run_exports, -1 from me. If you can do the leg work to fix all the recipes before merging this, I'm okay with this.

How about we solve this with a separate output rather than requiring recipes which only need header-only boost to add the right ignore_run_exports - the latter is IMO way less obvious and more error-prone.

In contrast, if a recipe declares a host-dependence on (say) libboost-header-only, then it would be apparent at first sight what that recipe is claiming to rely on, and why that wouldn't result in a run-export.

(yes, I'm thinking about eventually renaming boost-cpp -> libboost, and would be willing to do the legwork; it's <100 dependent feedstocks, so it's feasible with a bit of time)

PS. Having a header-only version would independently be useful e.g. to scipy (which vendors the headers), and arrow-cpp, see here, and AFAICT that would remove a bunch of packages from the boost-migration circuit, because IIUC the header-only dependencies wouldn't need to be pinned.

@jakirkham
Copy link
Member Author

Why not just add boost-hpp for the headers and use that? boost-cpp could depend on it.

@h-vetinari
Copy link
Member

Why not just add boost-hpp for the headers and use that? boost-cpp could depend on it.

Do I understand correctly that you think an additional header-only output is worth considering? The naming discussion can come after ;)

(I personally find boost-cpp & boost-hpp too close for comfort - plus the lib-prefix would be nice to have uniformly - but I agree "hpp" is a neat idea.)

@isuruf
Copy link
Member

isuruf commented Nov 2, 2022

AnacondaRecipes/boost-feedstock have all of these. What we have to do is to sync this recipe with that.
Only major thing we have to do is to write a migrator to go from boost-cpp -> libboost or boost-headers depending on if run had boost-cpp or not.

Nvm. there's no boost-headers or something similar there.

@h-vetinari
Copy link
Member

Nvm. there's no boost-headers or something similar there.

Yeah, from what I could tell, boost-cpp only has one output (and the boost feedstock is for the python bindings based on boost-cpp).

AnacondaRecipes using libboost is actually another good argument for doing the rename...

Only major thing we have to do is to write a migrator to go from boost-cpp -> libboost or boost-headers depending on if run had boost-cpp or not.

Might be a good exercise to do that (not sure how much effort it is to write a custom migrator), but based on the experience from smaller-scale renames with abseil & grpc, it would be possible to:

  • add libboost (keeping boost-cpp as a compat output)
  • migrate both outputs to a new version
  • work through the feedstocks to ensure they all use the new output [<- if the migrator isn't feasible, I'd be willing to do that]
  • remove the compat output eventually

@h-vetinari
Copy link
Member

AnacondaRecipes/boost-feedstock have all of these.

I had misinterpreted this as an "OR", but in any case, https://github.com/AnacondaRecipes/boost-feedstock does both the lib and the python bindings. The names are different from ours also for the python bindings:

conda-forge Anaconda
lib boost-cpp libboost
header-only lib - -
python bindings boost py-boost

@jakirkham
Copy link
Member Author

Why not just add boost-hpp for the headers and use that? boost-cpp could depend on it.

Do I understand correctly that you think an additional header-only output is worth considering?

Yes

@h-vetinari
Copy link
Member

@isuruf: Only major thing we have to do is to write a migrator to go from boost-cpp -> libboost or boost-headers depending on if run had boost-cpp or not.

I opened an issue to coordinate how to actually do this: #137

@h-vetinari
Copy link
Member

Only major thing we have to do is to write a migrator to go from boost-cpp -> libboost or boost-headers depending on if run had boost-cpp or not.

There's now a functioning draft for this: regro/cf-scripts#1668

@h-vetinari
Copy link
Member

This has been done in conda-forge/boost-feedstock#164, closing this

@h-vetinari h-vetinari closed this Sep 27, 2023
@jakirkham jakirkham deleted the add_run_exports branch October 2, 2023 20:21
@jakirkham
Copy link
Member Author

Thanks Axel! 🙏

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants