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

opt-level = 0 + debug=false does not produce no-debuginfo bins #79

Open
brson opened this issue May 17, 2019 · 7 comments
Open

opt-level = 0 + debug=false does not produce no-debuginfo bins #79

brson opened this issue May 17, 2019 · 7 comments

Comments

@brson
Copy link
Contributor

brson commented May 17, 2019

cmake-rs tries to translate cargo settings to cmake settings, but there are two cases where the debuginfio settings are ignored. The first is opt-level=0 + debug=false. This picks the CMake Debug profile, which always has debuginfo on.

The second is opt-level=s + debug=true. This picks MinSizeRel, which always has debuginfo off.

Solving this in the general case looks hard. I have begun trying to do it by running the cmake configure step once with the -LA flags, to list the configured values of CMAKE_C_FLAGS_DEBUG (etc); modify those flags, then run the actual configuration.

I think it's a viable solution, but I haven't quite gotten there: https://github.com/brson/cmake-rs/tree/dev-nodebug

This is a problem in TiKV where we are turning off debuginfo to improve compile times, but RocksDB makes up a huge chunk of that debuginfo. cc tikv/tikv#4711

We could probably solve this one-off in librocksdb_sys, but it would seem better to solve it for everybody.

@brson brson changed the title opt-level = 0 + debug=false does not produce no-nebuginfo bins opt-level = 0 + debug=false does not produce no-debuginfo bins May 17, 2019
@alexcrichton
Copy link
Member

I'm not personally intimately familiar enough with cmake to know how to fix this myself, but your patch seems reasonable to me! I sort of wish we could just tell it what flags we want in the axes that we have, but having a workaround in the meantime should be fine. Want to send a PR for those changes?

@brson
Copy link
Contributor Author

brson commented May 17, 2019

Yeah, I'll send a patch when I get it working.

It may be reasonable to just translate rust's options directly to CMAKE_C_FLAGS_DEBUG, but I also don't know enough about CMake to know whether that is ok.

@spl
Copy link

spl commented May 22, 2019

I don't have an answer, but I just wanted to put this thought out there.

Currently, cmake-rs does this:

  • if opt-level ≡ 0 then CMAKE_BUILD_TYPE=Debug,
  • if opt-level ∈ {1, 2, 3} then:
    • if debug ≡ false then CMAKE_BUILD_TYPE=Release
    • if debug ≡ true then CMAKE_BUILD_TYPE=RelWithDebInfo
  • if opt-level ∈ {s, z} then CMAKE_BUILD_TYPE=MinSizeRel

Here's one alternative that gives debug a higher precedence:

  • if debug ≡ true then:
    • if opt-level ∈ {0, s, z} then CMAKE_BUILD_TYPE=Debug
    • if opt-level ∈ {1, 2, 3} then CMAKE_BUILD_TYPE=RelWithDebInfo
  • if debug ≡ false then:
    • if opt-level ∈ {0, 1, 2, 3} then CMAKE_BUILD_TYPE=Release
    • if opt-level ∈ {s, z} then CMAKE_BUILD_TYPE=MinSizeRel

Perhaps you could move the {s, z} around a bit, but I guess one shouldn't expect to get a minimized build with debug info compiled in, so as long as debug ≡ true, you won't get CMAKE_BUILD_TYPE=MinSizeRel.

@spl
Copy link

spl commented May 22, 2019

I think that the algorithm I suggested in #79 (comment) is a reasonable and simple change that would handle the corner cases mentioned by @brson, but I'm not sure if it's the best way to go in the long term.

If it's given that the goal of a -sys crate is to build a non-Rust library under the control of a Rust library, I think it makes sense to have Rust, via cmake-rs, decide certain aspects of building. The CMAKE_<LANG>_FLAGS and CMAKE_<LANG>_FLAGS_<CONFIG> seem to be good candidates for those aspects. Unfortunately, CMake does not appear to make it easy/desirable to do this, and the general advice I've read is to not override these but use CMAKE_<LANG>_FLAGS_INIT and CMAKE_<LANG>_FLAGS_<CONFIG>_INIT instead. Plus, you might run into conflicts with particularly picky CMakeLists.txt.

With that in mind, perhaps it would be useful to have cmake::Config functions that allow one to choose how to override certain variables, giving different mechanisms as arguments. The options could include:

  1. Don't override. That is, let CMake or the CMakeLists.txt decide.
  2. Override by letting cmake-rs choose a value that best fits the cargo settings.

@alexcrichton
Copy link
Member

Having more options seems reasonable to me! I'm not really sure which is the best way but having a few to try seems reasonable.

@brson
Copy link
Contributor Author

brson commented May 22, 2019

Thanks for the suggestions @spl

@brson
Copy link
Contributor Author

brson commented May 31, 2019

From the linked tikv thread, it may end up that we use debuginfo=1 for release mode. CMake itself doesn't seem to make any distinction between debuginfo levels, just on or off. And furthermore, by my recollection, cargo itself doesn't provide the debuginfo level to the build script, just on or off.

So for the tikv use case we may not be able to get what we want out of cargo and cmake-rs. (the equivalent of opt-level=3 + debuginfo=1 for cmake projects).

Unfortunately rocksdb is a big chunk of our binary and our debuginfo. If the above scenario happens we may just have to hack up or build script for our fork of rocksdb.

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

No branches or pull requests

3 participants