-
Notifications
You must be signed in to change notification settings - Fork 16
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
Fix CMake build for paths with single quotes and spaces #22
base: master
Are you sure you want to change the base?
Conversation
An example of the error produced:
|
## Proposed Changes A user on Discord reported build issues when trying to compile Lighthouse checked out to a path with spaces in it. I've fixed the issue upstream in `leveldb-sys` (skade/leveldb-sys#22), but rather than waiting for a new release of the `leveldb` crate, we can also work around the issue by disabling Snappy in LevelDB, which we weren't using anyway. This may also have the side-effect of slightly improving compilation times, as LevelDB+Snappy was found to be a substantial contributor to build time (although I'm not sure how much was LevelDB and how much was Snappy).
## Proposed Changes A user on Discord reported build issues when trying to compile Lighthouse checked out to a path with spaces in it. I've fixed the issue upstream in `leveldb-sys` (skade/leveldb-sys#22), but rather than waiting for a new release of the `leveldb` crate, we can also work around the issue by disabling Snappy in LevelDB, which we weren't using anyway. This may also have the side-effect of slightly improving compilation times, as LevelDB+Snappy was found to be a substantial contributor to build time (although I'm not sure how much was LevelDB and how much was Snappy).
I've realised this might need some more tweaking for paths that contain quotes. I'll check the behaviour and update accordingly |
@michaelsproul thank you, would be happy to accept this patch! |
4c8f24f
to
74ef2e9
Compare
Awesome! As I suspected my original solution was broken for paths containing I've had enough of build system wrangling for one day, but I can try fixing it for |
Thanks! The additional shell_escape dependency looks okay in this case! |
@skade Sorry to bump an ancient PR. I've finally gotten around to improving handling of paths with double quotes. I gave up on getting it to actually work – I suspect the upstream CMake build system can't handle it. Instead I've made the build script produce an explicit error highlighting what the problem is (a path containing double quotes). For the simple cases of spaces & single quotes, compilation now works! It would be awesome if you could merge this PR and cut a new release. I'm also happy to volunteer as a maintainer if you need more hands. You can check my record on open source, including contributions to |
Currently, building
leveldb-sys
at a path with spaces in it fails with an error about the C compiler being unable to compile a simple program. I isolated the cause as the directory arguments to-I
and-L
being passed unquoted. Quoting them allows builds to succeed for project paths with spaces in them.The issue only occurs when
snappy
is enabled, so to reproduce you need to:leveldb-sys
to a path name with spaces, e.g.~/space dir/leveldb-sys
cargo build --features snappy