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

configure: BITCOIN_SUBDIR_TO_INCLUDE: Improve compatibility with paths including space and multiline cpp output #5872

Closed
wants to merge 2 commits into from

Conversation

luke-jr
Copy link
Member

@luke-jr luke-jr commented Mar 9, 2015

This fixes a number of bugs in the BITCOIN_SUBDIR_TO_INCLUDE macro:

  • tr was deleting all 'n' and 'r' characters in addition to the backslash, due to too much escaping.
  • Escaped spaces in paths were ignored, parsing the space as a delimiter
  • Multiline output from cpp (used when there are a number of includes being listed) weren't handled correctly.

The new sed takes two steps:

  1. Translate escaped newlines into a space, to work with single lines.
  2. Search for the file we want and return the complete path for it.

@jgarzik
Copy link
Contributor

jgarzik commented Mar 11, 2015

WFM ACK. I need to re-review all the escapes to make I'm happy with it.

@theuni
Copy link
Member

theuni commented Mar 12, 2015

@luke-jr That's pretty hard to read so I'll take your word that it's correct. Can you verify that it's portable, though? Non-gnu sed (*bsd) tends to choke pretty easily.

@luke-jr
Copy link
Member Author

luke-jr commented Mar 12, 2015

Except for the -E option for extended regex syntax, I took care writing the rest avoiding anything that wasn't part of POSIX. As for the -E, it's been used in BFGMiner's configure script for a while including at least one BSD user testing it (and reporting other compatibility issues) - although I'm not certain it was triggered in that scenario. It would be best to have someone verify this on BSD before merging.

@luke-jr
Copy link
Member Author

luke-jr commented Mar 12, 2015

@koobs on freenode tested the sed expression on FreeBSD 9/10/current and confirmed the -E parameter is supported by 8.

@laanwj
Copy link
Member

laanwj commented Apr 15, 2015

Does it really need all this complexity? If so, I'd prefer something that is more written-out so easier to verify.
Not that I don't trust you to get this right, but other people may have to maintain it at some point.

@laanwj
Copy link
Member

laanwj commented May 27, 2015

Is there anyone that can review this?

@laanwj
Copy link
Member

laanwj commented Jun 10, 2015

I've tried in different ways to find reviewers for this, to no avail. No one can review the sed code as-is, let alone maintain it. Please try to find a simpler solution e.g. break it up into multiple, documented steps.

@jgarzik
Copy link
Contributor

jgarzik commented Sep 15, 2015

Sadly, agree with @laanwj

Closing due to failing to garner review.

@jgarzik jgarzik closed this Sep 15, 2015
@sipa sipa reopened this Jun 8, 2016
@luke-jr
Copy link
Member Author

luke-jr commented Jun 8, 2016

Added detailed comments and otherwise improved readability. @theuni and anyone else, can you review? Nothing has changed that should break BSD compatibility, but a BSD tester would be appreciated as well.

@theuni
Copy link
Member

theuni commented Jun 10, 2016

@luke-jr Sorry I didn't get to this before leaving. I'm going to have to clear my head, make peace with the universe, accept mortality, and review this craziness while fresh. But more likely, bored on a plane. The comments are a big help, thanks.

Though I have to say, I'm still uneasy relying on something so cryptic, even if it is correct.

@sipa
Copy link
Member

sipa commented Aug 25, 2016

What issues does this solve?

@theuni can you review?

@luke-jr
Copy link
Member Author

luke-jr commented Aug 25, 2016

Compatibility with more compilers, and build paths with spaces in them.

@laanwj
Copy link
Member

laanwj commented Sep 29, 2016

and build paths with spaces in them

That sounds fairly important.

@theuni
Copy link
Member

theuni commented Sep 29, 2016

Here's an alternative that I'm more comfortable with: theuni@2ef8a6f

That completely does away with bitcoin_subdir_to_include.m4, as it was only used to find bdb.

Instead of building, asking the compiler what the resulting path was, and adding it to the flags, instead just cut off the subdir that worked and concat it back with the include. Since it already build that way in configure, we can be sure that it will work again during build.

Is there any obvious disadvantage?

@luke-jr
Copy link
Member Author

luke-jr commented Sep 29, 2016

Besides being an ugly hack, I'm not sure if that will be practical to make compatible with absolute paths as needed for --bdb-include-dir (every Google result for whether absolute paths are acceptable in includes resulted in advice saying basically "don't do that, use -I")

@theuni
Copy link
Member

theuni commented Sep 29, 2016

I'm not sure what you mean, this wouldn't change the semantics at all.
Example:

Say bdb lives at /foo/bar/db/db_cxx.h

./configure BDB_CPPFLAGS=-I/foo/bar
or
./configure --bdb-include-dir=/foo/bar

configure tries a bunch of paths, and lands on this working combo:
test.cpp:

conftest.cpp:
#include db/db_cxx.h
+
cpp -I/foo/bar conftest

and

Now as a result, it exports:
BDB_LIB=db/
BDB_CPPFLAGS=-I/foo/bar

So the result is the same. I'm not sure what you mean by "whether absolute paths are acceptable", everything in the above example is standard usage.

@luke-jr
Copy link
Member Author

luke-jr commented Sep 29, 2016

I see, so you'd combine -I and #define to find the file. (It's certainly not standard to use a #define in an #include, or you'd not have to jump through 5 extra LOC to do it.. :p)

But your solution may still be less uglier than doing this right... especially if nobody is willing to review it.

@theuni
Copy link
Member

theuni commented Oct 4, 2016

@luke-jr Not that I'm unwilling to review, simply unable. That sed program needs its own unit tests :p. I just don't think it's the right tool for the job.

I don't like theuni@2ef8a6f either, but at least it's relatively straightforward.

@laanwj
Copy link
Member

laanwj commented Nov 2, 2016

Going to close this, as this has been open for more than a year and there is no real progress toward it getting merged.

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

Successfully merging this pull request may close these issues.

5 participants