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

Allow using system absl #56

Merged
merged 1 commit into from
May 10, 2020
Merged

Conversation

martinetd
Copy link
Contributor

I'm pretty sure you won't like this one (at the very least I don't! which means that the packaging work I did for it will likely stay in my tree, oh well...) but I don't see much way around it.
I don't think nixos (the distro I use on my mpd box) would allow using a subproject... I'm pretty sure debian and the likes frown upon that but I'm not sure honestly.

(offtopic rant: It's a shame nixos already has an absl lib but it's built with default c++11 target so not abi-compatible, that took me ages to understand :/)

Anyway, I'm clueless about c++, but it looks like absl really is mostly only used for tests. Would it make sense to have macro conditionals using it or not? That would mean package codes aren't what's actually tested so it's probably a bad idea, but I think it does kind of get in the way of world domination (err I mean #36)
Long story short please close this once you've read this rant :D

@joshkunz joshkunz self-assigned this May 8, 2020
@joshkunz
Copy link
Owner

joshkunz commented May 8, 2020

Thanks for the PR, and your interest in the project. Sorry you ran into issues packaging, I definitely want to encourage packaging, not suppress it 😁.

That said, I'm a little confused about what problem you're trying to solve here.

On a base level, I'm hesitant to support this, since it goes against Abseil's compatibility and release guidelines. The Abseil maintainers actually recommend that you build directly against the source, since like you mention, they do not maintain any ABI compatibility whatsoever. They explicitly don't support dynamic linking, and they strongly recommend against statically linking pre-built Abseil binaries. Plus, like you mentioned, there will be version skew between the system Abseil version, and the Abseil version used in testing. Like you mention it's pretty unfortunate that nix has a packaged Abseil at all.

I'm not very familiar with distro release processes, but from what I can tell, both Debian and Nix support submodules. See Debian gbp-buildpackage's --git-submodules option. I'm only slightly familiar with Nix syntax, but it looks like Nixpkg's fetchgit has a fetchSubmodules option. I think fetchFromGithub has a similar option. Anyways, it's a pretty common way to vendor code, so I'd be somewhat surprised if distro vendors prohibited it. Are you sure that you can't just clone the submodules, and build from the repo + submodule sources? E.g., the process described on the readme?

If for whatever reason submodules are not supported, and that's what is blocking you, I would much rather just vendor the Abseil sources in the release tarball. Now that we have binary releases this should be trivial. We can just create another release artifact that also contains the Abseil sources. To be honest, I might just do this anyways, since it will make it easier on users building from source.

If none of those options work, I'm OK with accepting a patch like this. Though I think it should be a special option, rather than the default as you've written it here. E.g. -Dunsupported_use_system_absl=enabled or something. That way users get a supported build by default.

Leaving this open in case these options won't work, and we want to go ahead with this patch.

@martinetd
Copy link
Contributor Author

I'm not very familiar with distro release processes, but from what I can tell, both Debian and Nix support submodules.

Well, is it possible? Yes, definitely. My problem isn't so much about the technology of submodule, even if fetchFromGithub didn't have an option it would have been possible to specify both gits to fetch, but that I don't like "embedding code" in general.

I'm probably just biased on this, but this is basically the same problem I have with rust npm and all other languages locking their dependencies at a given version: if there ever is an important bugfix on some lib (security or otherwise) it's neigh impossible to update everything that uses it.
Yes, I'm having a very hard time in 2020 ;)

After some digging, a few references:
https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling
https://wiki.debian.org/UpstreamGuide#No_inclusion_of_third_party_code
(couldn't find anything for nixos but my gut feeling is the same, you can if you really have to but it's not recommended)

But anyway I agree this is probably not a good idea here. I didn't know abseil's stance on this but it definitely makes sense; things could be manageable in nixos' case as it's easy to have different binary versions of absl in parallel and specify which you want, but for a normal distro it would be downright impossible to link with the systems packages.

BTW from a regular user perspective the submodule is checked out by meson automatically so I don't think it's a problem that warrants embedding the code. Had I continued to use ashuffle from my home I would have had no problem building at all.
You'll probably have an easier time updating abseil once in a while as things are now so I'd keep it that way -- better a submodule than source files dangling about...
(And I'm not that mad to have changed the default, my commit still builds from submodule by default ;) But sure I can change option name if that's your only problem with it, I'm not fussy on this particular patch though, just wanted to let some steam off :p Thanks for taking it seriously!)

Copy link
Owner

@joshkunz joshkunz left a comment

Choose a reason for hiding this comment

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

Other than the option name, this LGTM. Thanks for the explanation.

Most distros don't like projects embedding subprojects, so
we need to be capable of building using the system's absl.
It is not going to work easily because the system's absl
needs to be built with the C++17 standard to work, so
ultimately this might not be such a good idea, but it
works for me on nixos...
@joshkunz joshkunz merged commit c7dd1da into joshkunz:master May 10, 2020
@joshkunz
Copy link
Owner

Tagged v3.2.0

@martinetd
Copy link
Contributor Author

Woops, sorry for the syntax error I should have checked travis.
Thanks!

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.

2 participants