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

luke-jr commented at 6:39 pm on March 9, 2015: member

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:

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

configure: BITCOIN_SUBDIR_TO_INCLUDE: Improve compatibility with paths including space and multiline cpp output 49f4f99d4f

jgarzik commented at 1:53 pm on March 11, 2015: contributor

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

theuni commented at 4:18 am on March 12, 2015: member

@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 commented at 9:05 am on March 12, 2015: member

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.

laanwj added the label Build system on Mar 12, 2015

luke-jr commented at 10:17 am on March 12, 2015: member

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

laanwj commented at 11:02 am on April 15, 2015: member

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 commented at 11:03 am on May 27, 2015: member

Is there anyone that can review this?

laanwj commented at 8:21 am on June 10, 2015: member

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 commented at 6:02 pm on September 15, 2015: contributor

Sadly, agree with @laanwj

Closing due to failing to garner review.

jgarzik closed this on Sep 15, 2015

BITCOIN_SUBDIR_TO_INCLUDE: Improve compatibility further, and reformat to be more readable e856193daf

sipa reopened this on Jun 8, 2016

luke-jr commented at 10:44 pm on June 8, 2016: member

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 commented at 9:12 pm on June 10, 2016: member

@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 commented at 11:42 am on August 25, 2016: member

What issues does this solve? @theuni can you review?

luke-jr commented at 5:22 pm on August 25, 2016: member

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

laanwj commented at 7:22 am on September 29, 2016: member

and build paths with spaces in them

That sounds fairly important.

theuni commented at 5:20 pm on September 29, 2016: member

Here’s an alternative that I’m more comfortable with: https://github.com/theuni/bitcoin/commit/2ef8a6fc3b1de989702adb04ebb066bd101350f0

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 commented at 7:39 pm on September 29, 2016: member

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 commented at 9:30 pm on September 29, 2016: member

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:

0conftest.cpp:
1#include db/db_cxx.h
2+
3cpp -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 commented at 9:39 pm on September 29, 2016: member

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 commented at 3:43 pm on October 4, 2016: member

@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 https://github.com/theuni/bitcoin/commit/2ef8a6fc3b1de989702adb04ebb066bd101350f0 either, but at least it’s relatively straightforward.

laanwj commented at 2:17 pm on November 2, 2016: member

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

laanwj closed this on Nov 2, 2016

MarcoFalke locked this on Sep 8, 2021

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