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

Run exports #18

Closed
wants to merge 16 commits into from
Closed

Run exports #18

wants to merge 16 commits into from

Conversation

isuruf
Copy link
Member

@isuruf isuruf commented Mar 4, 2018

@ocefpaf, we need to do this for all the packages in conda-forge-pinning yaml file.

@mingwandroid, is it okay to add you here?

recipe/meta.yaml Outdated
number: 2
run_exports:
# https://abi-laboratory.pro/tracker/timeline/zlib/
- {{ pin_subpackage('zlib', max_pin='x.x.x') }}
Copy link
Member

Choose a reason for hiding this comment

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

The past 4 releases have been fine. Why pin so tightly? With zlib so low in the stack, it'll be very painful to rebuild with the next version - especially if it's actually totally compatible and would otherwise be fine.

Our experience with this has been that it is better to give packages the benefit of the doubt. You can either optimize for users never breaking (pin tightly, like here), or optimize for package builder time (pin as loosely as reasonably achievable). Since you'd have to rebuild if something breaks, anyway, you're primarily just making work for yourself by pinning tightly.

Copy link
Member

Choose a reason for hiding this comment

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

As an example of the types of problems seen, please this diff report between 1.2.8 and 1.2.9. Note they changed a struct and it will affect public API functions. This happened in a relatively late patch release. Admittedly 1.2.10 merely changed the copyright and 1.2.11 was compatible with 1.2.10. However these were released within days of each other. The history before 1.2.8 is more unsettling. Also please see this discussion for more details.

Copy link
Member

Choose a reason for hiding this comment

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

That issue does not account for forward compatibility, which has been the trend. Comparing the history before 1.2.8 isn't really enlightening, because it comes from 2013. Current development has all been fine. Adding symbols is OK. That's the nature of forward compatibility.

@mingwandroid
Copy link
Contributor

@jakirkham, Changing the internal_state structure does not change the public API here. Yes it's in the zlib.h header but it's not passed as a parameter to nor returned by in any functions in the header so nothing would be able to get access to any instances of this structure.

@conda-forge-linter
Copy link

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

I wanted to let you know that I linted all conda-recipes in your PR (recipe) and found some lint.

Here's what I've got...

For recipe:

  • Failed to even lint the recipe (might be a conda-smithy bug) 😢

@jakirkham
Copy link
Member

cc @dougalsutherland

@carlodri
Copy link

carlodri commented May 2, 2018

should we also add the --static flag to compile also the statically linked library? I think that this could be the reason for which ffmpeg builds (conda-forge/ffmpeg-feedstock#47) fail to find zlib.

@msarahan
Copy link
Member

msarahan commented May 2, 2018

If you do that, you should make a different package output for it. Static libs should have their own packages, so that they can be made to not use run_exports, while allowing the shared lib packages to use run_exports.

@carlodri
Copy link

carlodri commented May 2, 2018

thanks @msarahan, so you mean a zlib-dev (shared) and a zlib (static) package, and a corresponding ffmpeg-dev and ffmpeg?

@msarahan
Copy link
Member

msarahan commented May 2, 2018

something like that, although the community already has a very well-established custom of the plain package name being mostly for shared. so perhaps zlib-static. Anyway, I'm going to leave that here, because it's a massive invitation for bikeshedding. If you'd like to bring that up for community discussion, we can invite you to the next conda-forge call.

@mingwandroid
Copy link
Contributor

mingwandroid commented May 2, 2018

Sorry @msarahan, I agree a meeting would be great and to bikeshed this a little more we may consider discussing in that meeting that something like {{ shared_linkage('zlib') }}/{{ static_linkage('zlib') }} but most importantly {{ default_linkage('zlib') }} would allow the recipe to be the same for normal usage and also for some tin-foil hat wearers to set default to 'static' because 'shared' linking is an invention of $some_evil_deity or $_some_3_letter_agency or other.

@jakirkham
Copy link
Member

Ok, a few things worth noting.

  1. zlib already contains the static library. We even test for it.
  2. Including shared and static libraries in packages in conda-forge is by far the norm and has been for some time.
  3. Dropping things from packages will likely upset people (this has come up before).

We can of course discuss this more in a meeting. Expect whatever solution we come up with will be some form of split packages. Generally would advise giving some thought to issue. ( conda-forge/conda-forge.github.io#544 ) Whatever sugar syntax we use will likely need to know how we chose to split things, which means systematizing splitting.

@carlodri
Copy link

carlodri commented May 2, 2018

@msarahan @jakirkham sorry didn't want to stir up a hornets' nest! My goal was simply to understand which is the best way to contribute and respect/define good and recognized standards. However, being quite a novice in C/C++ build toolchains/compiler/linker stuff (and in conda-stuff too) I don't think I can give too much advice on the matter.

zlib already contains the static library. We even test for it.

right, sorry, didn't see that! I wonder why the --shared flag then? It seems that a simple ./configure builds both shared and static by default (ref). But ok, if it works, don't fix it...

@isuruf isuruf closed this May 25, 2018
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.

7 participants