conda-forge / libxcb-feedstock

A conda-smithy repository for libxcb.
BSD 3-Clause "New" or "Revised" License
0 stars 10 forks source link

Provide Libtool's `.la` files once again. #9

Closed pkgw closed 5 years ago

pkgw commented 5 years ago

I had intended not to include these files in the original package, but due to a typo, they had been included. As alluded to in the code comment in build.sh, removing these files turns out to be a breaking change that has been messing up builds. At the moment, the most sensible course of action seems to be to go back to providing these. However, we start emulating Debian by blanking out the dependency_libs item in the .la files, which causes the interdependencies and is not needed now that virtually everyone uses pkg-config.

Checklist

conda-forge-linter commented 5 years ago

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.

mingwandroid commented 5 years ago

You need to eradicate .la files from the stack starting from the ground up.

It would be nice to synchronise with changes made to: https://github.com/AnacondaRecipes/libxcb-feedstock/tree/master/recipe

pkgw commented 5 years ago

Ah, looks like you've merged libxcb and xcb-proto? Among other changes. There isn't some kind of mechanism for GitHub to "link" this repo with the AnacondaRecipes one in a way that will automate the process of keeping tabs on what happens in the latter, is there? I have to admit that I'm pessimistic that folks will do a good job of monitoring changes in another repo without some kind of automated system to prod them.

I would like conda-forge to get rid of the .la files, but I wouldn't say we have consensus on that. It will probably be a bit of a painful process if we do it; as far as I can see we don't have very good tooling for analyzing the contents of the packages that we produce in PRs and whatnot. (Just a tar tjf |sort of the resulting package file would, I think, be very helpful to include in the CI output.)

pkgw commented 5 years ago

@jakirkham @ocefpaf @isuruf CI passes, at least. Any comments on this one?

mingwandroid commented 5 years ago

with dependency_libs emptied seems like a bad idea to me, you prevent anyone who wants or has to use libtool from linking to your static lib. You should either get rid of them entirely or provide them but without breaking them in this regard.

mingwandroid commented 5 years ago

@jakirkham @ocefpaf @isuruf please decline to merge a PR that infects other packages with incorrect linking information. Fix the real problem @pkgw.

pkgw commented 5 years ago

Well, I have preserved the "fix" of not providing the static libraries, so that particular angle is moot. Just emulating Debian here: staged-recipes issue, Debian policies.

mingwandroid commented 5 years ago

@pkgw, the ultimate goal (at least for me) is for our recipes to disappear and be direct forks conda-forge ones (where possible).

I'm not really that concerned with what Debian does, please stop pointing it out to me because I don't hold that distro in high regard, but for the record, from Debian policies:

10.2 Libtool .la files should not be installed for public libraries. If they’re required (for libltdl, for instance), the dependency_libs setting should be emptied. Library packages historically including .la files must continue to include them (with dependency_libs emptied) until all libraries that depend on that library have removed or emptied their .la files.
mingwandroid commented 5 years ago

Well, I have preserved the "fix" of not providing the static libraries, so that particular angle is moot. Just emulating Debian here

.. and you are sure that consumers of the newly brought back .la files will handle them correctly (and worse, not propagate any issues to their consumers, for that's what .la files tend to do)? (with all the complexity around how libtool gets built and/or bundled by different projects).

pkgw commented 5 years ago

I'm definitely not 100% sure that the new .la files will work correctly, but the failure mode is that builds of various XCB-depending libraries continue to fail on the conda-forge build system. If that happens, we can iterate. I'm just a volunteer and there's only so much time I can/should devote to this stuff, and conda-forge just doesn't have the infrastructure to do large-scale, "rebuild the world"-type testing.

I can't respond to not holding Debian "in high regard" with a technical argument, but I believe that it's worth paying attention to their policies since their project has literally decades of experience in consensus-driven, community-based packaging of Linux software. The cross-platform nature of conda-forge is a significant difference, but when it comes to things like how to deal with Libtool, I think their prior art is worth considering — and in this case, their policies make sense based on my understanding of the relevant software landscape. FWIW, the point of this PR is exactly encapsulated in the quoted sentence:

Library packages historically including .la files must continue to include them (with dependency_libs emptied) until all libraries that depend on that library have removed or emptied their .la files.

mingwandroid commented 5 years ago

It is not the case that there is a large stack to rebuild here, you build this then harfbuzz.

mingwandroid commented 5 years ago

I can't respond to not holding Debian "in high regard" with a technical argument, but I believe that it's worth paying attention to their policies since their project has literally decades of experience in consensus-driven, community-based packaging of Linux software. The cross-platform nature of conda-forge is a significant difference, but when it comes to things like how to deal with Libtool, I think their prior art is worth considering — and in this case, their policies make sense based on my understanding of the relevant software landscape. FWIW, the point of this PR is exactly encapsulated in the quoted sentence:

Yes, and years of techincal debt too which I consider this .la stuff to be a part of. ArchLinux just destroys them (and rebuilds dependencies), no fuss no muss.

ocefpaf commented 5 years ago

@mingwandroid and @pkgw I'm not sure I'm qualified to say something here. You both are experts on what you say and I trust both opinions. However, since you are in disagreement, I'd like to find a path to unblock this.

I'm used to remove all the .la files and in the past these files, coming from defaults caused some issues to us due to hardcoded paths. I'm not sure if they are harmful if they have the place_holder_path and if conda is updating that correctly though.

pkgw commented 5 years ago

In defaults at least some of the problems were due to renames that left files referencing a lib64 directory that should have been just lib. That was a while ago, though, so I presume those have been ironed out. I don't recall seeing any problems related to the prefix rewriting.

Overall I do prefer the approach of getting rid of the .la files, but it will be a new piece of policy that is going to be tedious to enforce. Probably only a few package rebuilds will be needed to unstick things like cairo, though.

ocefpaf commented 5 years ago

In defaults at least some of the problems were due to renames that left files referencing a lib64 directory that should have been just lib.

Yes, and some hardcoded paths to some local machines.

That was a while ago, though, so I presume those have been ironed out. I don't recall seeing any problems related to the prefix rewriting

Probably but we still have some rm *.la in some of our feedstocks.

Probably only a few package rebuilds will be needed to unstick things like cairo, though.

@mingwandroid some differences between conda-forge and defaults are unavoidable because the community may want to go to different places. I wonder if there is something we can do here, like multiple outputs where the package-static would have the .a and .la files that @pkgw needs.

pkgw commented 5 years ago

@ocefpaf That's something to think about, but if it were up to me, I'd rather just remove the .la and (most of) the .a files from our packages altogether.

(I guess basically this PR was conceived in the context of getting back to last week's status quo to unbreak a few builds, but it seems we've re-started the discussion about the overall policy that was begun in staged-recipes#673.)

isuruf commented 5 years ago

@ocefpaf That's something to think about, but if it were up to me, I'd rather just remove the .la and (most of) the .a files from our packages altogether.

Then let's just remove the .la files libxcb-render.la, libxcb-xinput.la. From what I can tell, cairo, harfbuzz issues were because these two were left.

ocefpaf commented 5 years ago

Then let's just remove the .la files libxcb-render.la, libxcb-xinput.la. From what I can tell, cairo, harfbuzz issues were because these two were left.

If that is a solution @pkgw is happy with let's do that!

pkgw commented 5 years ago

(revised several times:)

@isuruf @ocefpaf Sadly that's not quite right. The build failures here are because of other .la files referencing ones that Libtool then blindly expects will exist.

For instance, in harfbuzz there's a problem because libharfbuzz links to libXrender, and xorg-libxrender provides its own libXrender.la file that references $PREFIX/lib/libxcb.la. Because libtool is dumb, the nonexistence of this referenced file causes it to fail.

To fix the problem you need to rebuild that package (that is, xorg-libxrender) and remove its .la files, even though libxcb was the package that changed. That's why it's a hassle. Removing the .la files low in the dependency stack, as I did in the latest rebuild here, breaks builds for a lot of packages higher in the stack. (So if some other package has an .la file referencing libXrender.la, my "fix" of xorg-libxrender has then broken builds that link against that package, etc.)

Again, for the packages that we've noticed problems with, probably not that much needs to be changed — but because libxcb is pretty low in the dependency stack, the recovery is likely to be relatively tedious.

(This need for depth-first rebuilds is why the Debian guidelines say [emph added] "packages historically including .la files must continue to include them ... until all libraries that depend on that library have removed or emptied their .la files.")

ocefpaf commented 5 years ago

(This need for depth-first rebuilds is why the Debian guidelines say [emph added] "packages historically including .la files must continue to include them ... until all libraries that depend on that library have removed or emptied their .la files.")

I'm OK with this as a temporary solution. We may ask @justcalamari to get a sub-graph of the packages that have .la so we can remove and rebuild them in the order.

pkgw commented 5 years ago

Yeah, if we want to adopt a policy of excluding .la files, that would be the approach to take, I think. The other bit would be some mechanism in staged-recipes so that we can ensure that new .la files don't sneak in — I don't believe that we have infrastructure to support that kind of vetting at the moment.

ocefpaf commented 5 years ago

Indeed we should add inspect the packages and warn/error when .la are present.

mingwandroid commented 5 years ago

There isn't any big dependencies chain to rebuild here. Please do that.

bgruening commented 5 years ago

Great discussion! Learned a lot :)

mingwandroid commented 5 years ago

we can ensure that new .la files don't sneak in

I believe conda-build should either delete them unconditionally or have an option to do so that defaults to enabled. Happy to make a PR for that if not one beats me to it?

pkgw commented 5 years ago

OK, I am not hearing anyone argue for keeping the .la files, so I am going to close this PR and submit a few new ones to remove the .la files from libraries that reference libxcb. Please holler if you think some other approach should be taken.

isuruf commented 5 years ago

@pkgw, thanks for doing that. Can you remove the .la files in this package that were accidentally left in?

@mingwandroid, can you remove .la files from libxcb in defaults, so that conda-forge and defaults are compatible?

mingwandroid commented 5 years ago

Yes, just after I change CB to delete them for me!

pkgw commented 5 years ago

@isuruf Yeah, that's one of the PR's I'll submit

pkgw commented 5 years ago

OK, we've got a first batch of PRs through CI if anyone cares to review ...

jakirkham commented 5 years ago

I guess the only question I have is do we need .la for static libraries? There are some people I know that like using static libraries from conda-forge for their own stuff. So if the .la files are important for this, it would be nice to have them somehow. If that's not the case, I won't worry about it.

pkgw commented 5 years ago

@jakirkham To the best of my understanding, pkg-config has pretty much eliminated that use case. But I certainly don't worry much about static libraries these days so take that with a grain of salt.