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

Refine build process #10

Merged
merged 3 commits into from
May 9, 2020
Merged

Conversation

kulp
Copy link
Collaborator

@kulp kulp commented May 9, 2020

The current pull request introduces a few improvements to the build process:

More details are provided in each commit message.

A Travis build succeeds.

kulp added 3 commits May 9, 2020 10:28
`build.rs` was attempting to avoid re-running `bindgen` by checking
modification times, but this prevented rerunning `bindgen` under some
circumstances when `build.rs` itself was changed (for example, to
introduce `bindgen` symbol whitelisting). Use Cargo's built-in
functionality to determine whether a rebuild should be done, both using

    println!("cargo:rerun-if-changed={}", path.display());

and using `bindgen::CargoCallbacks`, which necessitates updating
`bindgen` to version 0.53.
This avoids depending on Make directly, and paves the way for smoother
support of platforms like Windows.

Using `cargo build -vv` demonstrates that `cc` already takes care of
linking in the `lightningsys` library, so we do not need to manage that
ourselves any longer.

`cargo build -vv` also shows that a dependency on `lightning-sys.h` is
automatically generated, so we do not need to handle that one ourselves:

    [lightning-sys 0.2.0] cargo:rerun-if-changed=./C/lightning-sys.h

and `register.c` is unconditionally compiled into `liblightningsys.a`
whenever `build.rs` is rerun (this seems unworthy of optimization for
the time being, given how tiny `register.c` is).
It appears from rust-lang/rust#35061 and from [some build failures][1]
that position-independent executables are being built (see "-pie" flag
in [1]'s build output), which would require the objects in
`liblightning.a` to be built with `-fPIC` on x86_64, which is not
desirable.

Static linking would be more desirable in the long term, since it could
enable extra optimizations (`-flto` particularly) and generate better
code, but for now, switch to dynamic linking until a portable way
becomes available to ensure linking succeeds statically.

[1]: https://travis-ci.org/github/kulp/lightning-sys/jobs/685111507
@kulp
Copy link
Collaborator Author

kulp commented May 9, 2020

I hope also to propose some whitelisting changes to reduce the number of needlessly-generated bindings, and those changes could be considered "build refinements" that belong in the current pull request, or they could be separated into their own PR. Please let me know which is preferable.

@kulp
Copy link
Collaborator Author

kulp commented May 9, 2020

Also, I do not know what was wrong with Travis builds, but given that I have seen success since switching to dylib linking, perhaps the current PR fixes #6.

@petelliott
Copy link
Owner

I hope also to propose some whitelisting changes to reduce the number of needlessly-generated bindings, and those changes could be considered "build refinements" that belong in the current pull request, or they could be separated into their own PR. Please let me know which is preferable.

A seperate PR would be preferable

Also, I do not know what was wrong with Travis builds, but given that I have seen success since switching to dylib linking, perhaps the current PR fixes #6.

I'll re-enable Travis builds

@petelliott petelliott merged commit 9587845 into petelliott:master May 9, 2020
@kulp kulp deleted the build-refinements branch May 9, 2020 19:34
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