Similar to llvm-config, gets command-line flags that are needed to build
applications linking against StreamExecutor.
Details
Diff Detail
Event Timeline
streamexecutor/tools/streamexecutor-config/streamexecutor-config.in | ||
---|---|---|
63 | Can we scope the try/except around tokens.index? | |
88 | Consider description=__doc__ and then just updating this file's docstring. | |
130 | Nit, write to stderr (print('foo', file=sys.stderr))? (Actually not quite a nit because of how this program is going to be used -- ideally we wouldn't print anything other than flags to stdout.) | |
131 | Exit with an error code? Same elsewhere. | |
162 | Should we do something to tokenize the flags? (So that e.g. -Idir with spaces shows up correctly?) |
- Respond to jlebar's comments
streamexecutor/tools/streamexecutor-config/streamexecutor-config.in | ||
---|---|---|
130 | Since I'm using sys.exit here now, I'm just passing the error message to sys.exit instead and that will print it to stderr as well as returning error code 1. | |
162 | Great catch! I added explicit quotes around @CMAKE_INSTALL_PREFIX@ everywhere it is used. This made it slightly more complicated to check whether a token being added by SE was already present in the LLVM output, so I added a new function has_token to do the token escaping before doing the search. As for the output of llvm-config, I'll just trust that llvm-config quotes things correctly. |
streamexecutor/tools/streamexecutor-config/streamexecutor-config.in | ||
---|---|---|
155 | How does @CMAKE_INSTALL_PREFIX@ work? That is, who's responsible for replacing it with an actual path? From the example in the docstring, it looks like nobody is? |
streamexecutor/tools/streamexecutor-config/streamexecutor-config.in | ||
---|---|---|
155 | This is a cmake built-in variable that is replaced at build time when this file is configured. The code in this patch that sets up this configuration is the cmake configure_file command in line 2 in streamexecutor/tools/streamexecutor-config/CMakeLists.txt. |
streamexecutor/tools/streamexecutor-config/streamexecutor-config.in | ||
---|---|---|
155 |
...wow. Okay. What happens if the install prefix contains the ' character? Do we just not care? |
streamexecutor/tools/streamexecutor-config/streamexecutor-config.in | ||
---|---|---|
148 | I wonder if it's worth automatically picking out the path to llvm-config using @LLVM_BINARY_DIR@ instead of relying on it being on PATH. Makes things slightly easier for the user, and reduces the risk that they have a different LLVM on their PATH to the one that SE was built against. |
- ESCAPE_QUOTES for cmake configure_file
streamexecutor/tools/streamexecutor-config/streamexecutor-config.in | ||
---|---|---|
155 | Oh, thanks for reminding me. I added the ESCAPE_QUOTES option to the cmake configure_file command. That should make sure any quotes in the variable are properly escaped. |
OK. The python code '\"' is equivalent to the string containing one character, double-quote. So I think this is good so long as ESCAPE_QUOTES also escapes backslashes as \\ (which we care about because Windows filenames contain backslashes).
It turns out that ESCAPE_QUOTES doesn't do the right thing for Python because it is only meant for C string literals. So, for example, it doesn't escape single quotes. So I removed the ESCAPE_QUOTES option for cmake configure_file and instead wrapped the substitution in triple double quotes. This will still break if the path contains triple double quotes, but I'm willing to let that happen.
Between this change and the change to using LLVM_BINARY_DIRECTORY for the llvm-config path, jlebar, I think you might want to take another quick glance to make sure things still look OK.
streamexecutor/tools/streamexecutor-config/streamexecutor-config.in | ||
---|---|---|
150 | I think that's a good idea. I made that change and added another option to let the user override the llvm-config path if the configuration somehow goes wrong. |
Maybe I'm doing something wrong, but the single-quotes wrapping the include/lib paths given by --cppflags and --ldflags are not working for me. The compiler fails to find the StreamExecutor headers and libraries, and warns that the library directory doesn't exist. It's almost as if the quotes are being taken as part of the paths.
If I manually remove those quotes from the streamexecutor-config script then it works fine. It also works if I manually copy the output into the compiler command-line, so maybe there's some weird issue with how the quotes are handled by the shell when substituting command output with $()?
Tried on both OS X and Ubuntu, with a trivial program that just includes StreamExecutor.h.
I can verify that the quotes are breaking it, and reading through the shell docs shows that the shell doesn't process quotes after command substitution, so I guess the quotes are being passed as literal characters in those arguments. I have an idea of how to fix this, and I should have a patch posted soon.
Well, it turns out that the current llvm-config does not handle directories with special characters. I built llvm with -DCMAKE_INSTALL_PREFIX="$HOME/private/dir 'with' spaces" and installed it there. Then when I ran llvm-config --cppflags I got the following output:
-I/usr/local/google/home/jhen/private/dir 'with' spaces/include -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
where we can see that the spaces and single quotes have not been escaped.
I think we should mimic the llvm-config behavior because otherwise there will be two separate ways that paths are formatted in the output, and if anyone ever does make a workaround for llvm-config, it won't work for streamexecutor-config.
So I would say that even though it's kind of pathetic, I think we should not try to escape special characters in our output either.
I think we should mimic the llvm-config behavior because otherwise there will be two separate ways that paths are formatted in the output, and if anyone ever does make a workaround for llvm-config, it won't work for streamexecutor-config.
Sounds good to me.
So I would say that even though it's kind of pathetic [...]
You can always put a passive-aggressive comment in our source. :)
Can we scope the try/except around tokens.index?