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

Add Vendored Libdbus Support #408

Merged
merged 1 commit into from Dec 28, 2022
Merged

Add Vendored Libdbus Support #408

merged 1 commit into from Dec 28, 2022

Conversation

landhb
Copy link
Contributor

@landhb landhb commented Dec 24, 2022

This PR adds vendoring behind a feature flag, much like many other sys crates in the ecosystem.

  • Allows users to build without having libdbus-dev-1 installed. Since it will be compiled from source.
  • Allows users to cross compile with cross without needing a custom docker image.
  • Uses the cc crate directly, so neither cmake or meson are required for downstream users.

A downstream user would simply enable the vendored feature:

dbus = {version = "0.9.7", features = ["vendored"]}

And build with cross:

cross build --target x86_64-unknown-linux-musl

The PR is currently passing the existing test suite on x86_64:

cargo test --features vendored

@KillingSpark
Copy link

Without being an expert I am just wondering how the licensing works on vendored code, especially since some parts of libdbus are gpl only licenced?

Vendoring this might make it necessary to either strip all gpl Code out if possible and use the afl license or dual licence this as Apache 2 (if not using the vendoring flag) and gpl if using it.

@landhb
Copy link
Contributor Author

landhb commented Dec 24, 2022

I'm definitely not a lawyer/expert either when it comes to license issues.

Best I could find is this answer:

https://softwareengineering.stackexchange.com/questions/240633/is-licensing-an-issue-for-git-submodules

Which makes it sound like it's not much of an issue.

But your suggestion to use GPL under this feature is probably the safest option.

Copy link
Owner

@diwic diwic left a comment

Choose a reason for hiding this comment

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

Hi @landhb and thanks for the PR! This seems like a big improvement in usability for the cross compilation use case and I really like that :-)

Apart from the review comments below, I think we should also add a Github CI/workflow for this so it doesn't break without anyone noticing.

libdbus-sys/build.rs Outdated Show resolved Hide resolved
libdbus-sys/build.rs Outdated Show resolved Hide resolved
.gitmodules Show resolved Hide resolved
libdbus-sys/build.rs Outdated Show resolved Hide resolved
@landhb
Copy link
Contributor Author

landhb commented Dec 24, 2022

@diwic No problem! 100% agreed on the testing, I left the PR in draft form since I also wanted to add this to CI/CD.

I also need to make sure it builds on multiple architectures. So far I've only tested x86_64.

I'll try to work through the changes + testing over the next few days.

@diwic
Copy link
Owner

diwic commented Dec 24, 2022

@KillingSpark from https://github.com/freedesktop/dbus/blob/master/COPYING

"dbus is licensed to you under your choice of the Academic Free
License version 2.1, or the GNU General Public License version 2
(or, at your option any later version)."

From my layman's understanding, neither the AFL (which is a BSD-style license), nor the GNU GPL, make any significant differences depending on whether you link statically or dynamically.

@KillingSpark
Copy link

KillingSpark commented Dec 24, 2022

From my layman's understanding, neither the AFL (which is a BSD-style license), nor the GNU GPL, make any significant differences depending on whether you link statically or dynamically.

I don't know about BSD but I think for GPL it matters for the thing about being derivative work? If it's linked statically your binary is a derivative work but if it's linked dynamically it could be combined with any abi compatible library.

But just as you I am a layman. Just wanted to raise awareness that this might be a topic to think about :)

Edit: seems like there are different opinions on this. GNU seems to think that for GPL it doesn't matter if it's static or dynamic. For LGPL the situation is more or less as described above. Others seem to think the situation is as described above regardless of GPL or LGPL being used. So... well I dunno

@KillingSpark
Copy link

KillingSpark commented Dec 25, 2022

https://softwareengineering.stackexchange.com/questions/240633/is-licensing-an-issue-for-git-submodules

This discussion is not completely the same. This pr doesn't just use libdbus as a tool while building. I'd agree that that would be fine. It is pulling in and building the code and is necessary for using the library after it is built, making it, in my non-lawyer opinion, either a combined or derivative work.

Edit: just wanna make sure I don't come off too harshly, I love the idea of the PR it would definitely be valuable to have this feature! I just think we should Honor the licensing of these projects :)

@diwic
Copy link
Owner

diwic commented Dec 25, 2022

I don't know about BSD but I think for GPL it matters for the thing about being derivative work?

Again in layman terms:

If you pick GPL for libdbus, and you pick MIT for dbus-rs, then the entire application will most likely be a GPL derivative.
If you pick AFL for libdbus, and either APL or MIT for dbus-rs, then the entire application can be proprietary, open source, etc, because these licenses are not contagious.

How we link to the libdbus code does not change this.

(LGPL does make a difference between static and dynamic linking, but that does not matter here because libdbus is not licensed under LGPL, and neither is dbus-rs.)

@diwic diwic marked this pull request as ready for review December 27, 2022 15:39
@diwic diwic marked this pull request as draft December 27, 2022 15:41
@landhb
Copy link
Contributor Author

landhb commented Dec 27, 2022

@diwic The x86_64-unknown-linux-gnu cross build should be fixed now. That cross image has a much older gcc which didn't understand some of the pragma directives.

@landhb landhb marked this pull request as ready for review December 27, 2022 16:15
@diwic
Copy link
Owner

diwic commented Dec 27, 2022

@landhb Thanks, this is getting better and better :-)

Just to understand how this is all going to work for the end user, will the dbus source end up on crates.io and downloaded with the package? Or will cross somehow download sources from git?

Edit: Found the answer myself - the build system calls git.

Copy link
Owner

@diwic diwic left a comment

Choose a reason for hiding this comment

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

Okay, I hope this is the final set of review comments. After fixing, please squash the commits into one and I believe it should be ready for merging.

libdbus-sys/build_vendored.rs Show resolved Hide resolved
libdbus-sys/build_vendored.rs Show resolved Hide resolved
libdbus-sys/cross_compile.md Outdated Show resolved Hide resolved
libdbus-sys/cross_compile.md Outdated Show resolved Hide resolved
libdbus-sys/build_vendored.rs Show resolved Hide resolved
libdbus-sys/build_vendored.rs Show resolved Hide resolved
libdbus-sys/build_vendored.rs Show resolved Hide resolved
libdbus-sys/build_vendored.rs Show resolved Hide resolved
@landhb
Copy link
Contributor Author

landhb commented Dec 27, 2022

@landhb Thanks, this is getting better and better :-)

Just to understand how this is all going to work for the end user, will the dbus source end up on crates.io and downloaded with the package? Or will cross somehow download sources from git?

Edit: Found the answer myself - the build system calls git.

@diwic So my understanding is that publishing on crates.io archives the entire git submodule along with the source. Elaborated more here with libgit2-sys as an example.

So if users are using the crates.io version they won't need git. But developers who are working on this repo and have cloned it will either need to do a recursive clone/checkout or let build_vendored.rs check it out for them.

@diwic
Copy link
Owner

diwic commented Dec 28, 2022

@landhb
So I merged it, then just ran (locally here, from the workspace directory)

$ git pull --rebase origin
$ cargo test --features vendored
error: failed to run custom build command for `libdbus-sys v0.2.2 (/home/david/rust/dbus-rs/libdbus-sys)`

Caused by:
  process didn't exit successfully: `/home/david/rust/dbus-rs/target/debug/build/libdbus-sys-55ef2401f6ef29e5/build-script-build` (exit status: 1)
  --- stdout
  cargo:rerun-if-changed=build.rs
  cargo:rerun-if-changed=build_vendored.rs

  --- stderr
  Error: Os { code: 2, kind: NotFound, message: "No such file or directory" }

Inside libdbus-sys/vendor there is a single /dbus/config.h which is 0 bytes. Any ideas? Or is this only supposed to work with cross, not normal testing/compilation?

@landhb
Copy link
Contributor Author

landhb commented Dec 28, 2022

@diwic Looks like I typo'd the source path check. Submitted a fix here: #409

@landhb
Copy link
Contributor Author

landhb commented Dec 28, 2022

Thanks for all the review work @diwic 🎉

@diwic
Copy link
Owner

diwic commented Dec 28, 2022

@landhb Thanks for the contribution! Nice to see some improvement in this area :-)

I'll wait a few days to see if any bugs come up as a result of this PR, otherwise I'll prepare a release of libdbus-sys and dbus-rs. Sounds like a plan?

@landhb
Copy link
Contributor Author

landhb commented Dec 28, 2022

Sounds good to me!

@diwic
Copy link
Owner

diwic commented Dec 30, 2022

@landhb I notice tons of the following warnings in the CI output, anything to worry about?


libdbus-sys-linux (stable, aarch64-unknown-linux-musl)
The `set-output` command is deprecated and will be disabled soon. Please upgrade to using Environment Files. For more information see: https://github.blog/changelog/2022-10-11-github-actions-deprecating-save-state-and-set-output-commands/

@landhb
Copy link
Contributor Author

landhb commented Dec 30, 2022

@diwic Looks like it's the toolchain action: actions-rs/toolchain#221

Since we aren't explicitly using set-output.

Looks like dtonly has a drop in replacement now, that we can swap for:

https://github.com/marketplace/actions/rustup-toolchain-install

@landhb
Copy link
Contributor Author

landhb commented Dec 30, 2022

@diwic Started a pull here to upgrade some of those deps: #411

@diwic
Copy link
Owner

diwic commented Jan 6, 2023

@landhb Released now, enjoy!

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.

None yet

3 participants