Rework include file/path handling so we can install the compiler #265
Conversation
|
One major question here is whether the include files should be installed directly in $prefix ( /usr/local by default), or in a package specific directory (/usr/local/share/p4c/p4include)? |
frontends/common/options.cpp
Outdated
| char buffer[PATH_MAX]; | ||
| int len; | ||
| struct stat st; | ||
| if ((len = readlink("/proc/self/exe", buffer, sizeof(buffer))) > 0 || |
There was a problem hiding this comment.
Is this stuff portable on other operating systems?
Maybe it should be in a Linux-specific library. We haven't been too careful about this kind of system dependences so far.
There was a problem hiding this comment.
Its portable to Linux, *BSD, and Solaris, and it won't fail badly on other platforms. The fallback using argv[0] will work on OSX and I believe on Windows (cygwin/msys), though I haven't tried the latter
| } else { | ||
| buffer[0] = 0; } | ||
|
|
||
| if (char *p = strrchr(buffer, '/')) { |
There was a problem hiding this comment.
It seems that you are assuming that the include path shares the same prefix with the binary path.
Is this a common assumption?
There was a problem hiding this comment.
This is for running tests with a binary in the build directory -- iff there's a p4include directory in the same directory as the executable, it overrides the installed config directory.
frontends/common/options.cpp
Outdated
| @@ -145,8 +180,8 @@ FILE* CompilerOptions::preprocess() { | |||
| #else | |||
| std::string cmd("cpp"); | |||
| #endif | |||
| cmd += cstring(" -undef -nostdinc -I") + | |||
| p4includePath + " " + preprocessor_options + " " + file; | |||
| cmd += cstring(" -undef -nostdinc -I") + (isv1() ? p4_14includePath : p4includePath) | |||
There was a problem hiding this comment.
Shouldn't the p4includePath be appended at the end?
This way we cannot override it with -I.
There was a problem hiding this comment.
I left it unchanged from before, but you're correct, the standard directory should be last in the path (that's the way C compilers do it; gcc has a special -idirafter option to add directories after the standard set)
|
One other major question is whether we need a versioning scheme for included files. |
frontends/common/options.cpp
Outdated
| } else if (strchr(argv[0], '/')) { | ||
| getcwd(buffer, sizeof(buffer)); | ||
| strncat(buffer, argv[0], sizeof(buffer) - strlen(buffer) - 1); | ||
| } else if (getenv("_")) { |
There was a problem hiding this comment.
I actually don't know what getenv("_") is.
Maybe you can add a comment?
There was a problem hiding this comment.
Some shells (bash, ksh, and zsh) set the _ environment variable to the full path of the binary they're about to execute.
Makefile.am
Outdated
| #### For testing, install headers in the build directory | ||
|
|
||
| check-install-headers: | ||
| @$(MAKE) install-data prefix=. |
There was a problem hiding this comment.
should this be @$(MAKE) install-data prefix=$(builddir)?
There was a problem hiding this comment.
Possibly, though I'm not sure under what circumstances $(builddir) could be anything other than .
frontends/common/options.cpp
Outdated
| char buffer[PATH_MAX]; | ||
| int len; | ||
| struct stat st; | ||
| if ((len = readlink("/proc/self/exe", buffer, sizeof(buffer))) > 0 || |
There was a problem hiding this comment.
If I understand correctly this code is finding the directory from where the program was invoked. Why not use dirname(argv[0])? It is much more portable than using /proc. This implementation will not work on MacOS.
There was a problem hiding this comment.
dirname(argv[0]) is the fallback and only works if the program was invoked with an absolute path. We use cwd+dirname(argv[0]) for a relative path. We could add searching through the $PATH envvar for cases where there is no path in argv[0].
That's part of why I was thinking |
ChrisDodd
force-pushed
the
cdodd-stage
branch
3 times, most recently
from
a5b8144 to
dde4a26
Compare
- look for p4include in the same dir as the executable to override the install p4include path. - install includes in the build directory so tests run.
ChrisDodd
force-pushed
the
cdodd-stage
branch
from
23bbed7 to
ae0560f
Compare
install p4include path.