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 libmilter-dev for milter-sys #62

Merged
merged 1 commit into from
Nov 30, 2020
Merged

Add libmilter-dev for milter-sys #62

merged 1 commit into from
Nov 30, 2020

Conversation

glts
Copy link
Contributor

@glts glts commented Nov 29, 2020

This adds package libmilter-dev for the milter-sys crate.

Thank you!

@jyn514
Copy link
Member

jyn514 commented Nov 29, 2020

@glts it looks like that crate built successfully - is there a reason you're adding the new package? https://docs.rs/milter-sys/0.2.0/milter_sys/

@glts
Copy link
Contributor Author

glts commented Nov 29, 2020

Thanks for looking.

libmilter-dev is strictly required for building milter-sys under any circumstances. Why it did build on docs.rs I do not know, it is possible that libmilter was installed as a transitive dependency of something else.

@jyn514
Copy link
Member

jyn514 commented Nov 29, 2020

I think we had this discussion a while ago 😆 You ended up special casing docs.rs WRT the pkg-config file. rust-lang/docs.rs#191 (comment)

Is the idea here to let you get rid of the hack in the build.rs? I'm ok with adding it in that case, there should just be some reason.

@glts
Copy link
Contributor Author

glts commented Nov 29, 2020

@jyn514 hm … I have trouble understanding how this library is different from other -sys libraries to you. milter-sys is simply the low-level FFI bindings crate for libmilter. It cannot be built or doc’ed without libmilter-dev.

My motive for opening this pull request is to make it explicit that libmilter-dev must be installed in the docs.rs build env. It’s true that milter-sys docs can be built on docs.rs today, but it is unclear why. I would guess that there is some other library in the docs.rs build env that installs libmilter as a dependency. But such a hidden dependency may disappear and break in the future. I think it is better to be explicit about the libmilter-dev requirement, that’s all – please tell me if you think otherwise.

I do hope to also get rid of the build hack you mentioned, soon, but that is an independent change that is not affected by this pull request.

@jyn514
Copy link
Member

jyn514 commented Nov 29, 2020

It’s true that milter-sys docs can be built on docs.rs today, but it is unclear why. I would guess that there is some other library in the docs.rs build env that installs libmilter as a dependency. But such a hidden dependency may disappear and break in the future. I think it is better to be explicit about the libmilter-dev requirement, t

Ok, this is the bit I was missing, thanks :) but I'm still confused what's going on, because it's not installed in the build environment:

$ docker run -it rustops/crates-build-env bash
root@d85ecfbe8395:/# apt list --installed | grep milter  # no output

I have trouble understanding how this library is different from other -sys libraries to you.

The difference is that your crate is building successfully without this library installed, and the build image is already very large, so I'd like to avoid adding new libraries unless they're necessary.

@glts
Copy link
Contributor Author

glts commented Nov 29, 2020

Thanks, that really is curious. On any Ubuntu 20.04 or Debian Testing I’ve tried cargo build or cargo doc fails for milter-sys – until I apt install libmilter-dev. I will try to find out why it is working on docs.rs.

@glts
Copy link
Contributor Author

glts commented Nov 29, 2020

Sorry to bother you again, do you know if for building the docs, Cargo checks if a library links properly? I think it doesn’t (nor does it run any tests, of course). So in that case the C library is indeed not needed at all for the milter-sys docs, I misunderstood the purpose of packages.txt and should close this pull request!

@jyn514
Copy link
Member

jyn514 commented Nov 29, 2020

Right, cargo doc doesn't need to link the library at all and just type checks the code. Glad we figured it out! :)

@jyn514 jyn514 closed this Nov 29, 2020
@glts
Copy link
Contributor Author

glts commented Nov 29, 2020

@jyn514 Hang on, we still need this. I do apologise for all the confusion, today is not my best day. Yes, cargo doc doesn't need to link the library, but the build.rs will look for pkg-config metadata, once the build hack there is removed. You were right all along, sorry.

I would prefer if we could add libmilter-dev here, and I can remove the build hack to have a clean build in milter-sys. Let me know if you could agree to this.

@jyn514
Copy link
Member

jyn514 commented Nov 29, 2020

Ok, makes sense - then crater will be able to build milter-sys too.

@jyn514 jyn514 reopened this Nov 29, 2020
@glts
Copy link
Contributor Author

glts commented Nov 30, 2020

Great, thank you. I’ve removed the build.rs hack in milter-sys. I’ve confirmed locally that the docs.rs build would now fail (when I push a new release).

@jyn514 jyn514 merged commit 775e47f into rust-lang:master Nov 30, 2020
@glts
Copy link
Contributor Author

glts commented Dec 21, 2020

Could someone from the team please schedule a build for crate milter-sys, please? Thank you.

@pietroalbini
Copy link
Member

Scheduled a rebuild!

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

Successfully merging this pull request may close these issues.

3 participants