Makefile changes #33
Makefile changes #33
Conversation
for -g flag see [Issue 30](Stewori#30) the tests targets may not work but they print an error to that effect Moving and splitting out cleans should also work on all the systems
|
This is to do with issue 30 |
makefile
Outdated
|
|
||
| build-tests: | ||
| @echo 'building tests is not fully supported yet, this will either not work or try to install the demo extension as an actuall extension' | ||
| python ./DemoExtension/setup.py install |
There was a problem hiding this comment.
What would this actually do? I suspect this might install it to system's CPython. If that is the case I think we should better keep it locally in the JyNI folder. Would python ./DemoExtension/setup.py build be sufficient here? I think that's what I usually call to create DemoExtension.so (or DemoExtension.pyd).
There was a problem hiding this comment.
Sorry I had forgotten about that, I didn't realise python setup.py build was a thing. It seems to work in eclipse as well as the command line, I'll update the branch/pull request.
makefile
Outdated
| @@ -115,12 +129,25 @@ ifeq "$(wildcard $(JAVA_HOME) )" "" | |||
| $(eval JAVA_HOME = $(shell $(JAVA) -jar $(JYTHON) -c "from java.lang import System; print System.getProperty('java.home')[:-4]")) | |||
| endif | |||
|
|
|||
| # assume you want debug if you are just compiling one portion of the codebase | |||
| libJyNI: CFLAGS += -g | |||
| libJyNI: $(JAVA_HOME) $(OBJECTS) JyNI-C/src/Python/dynload_shlib.o | |||
There was a problem hiding this comment.
Interesting. I never really studied makefile syntax systematically. Does this really work as expected? I.e. with the double entry libJyNI:
There was a problem hiding this comment.
Yes, oddly enough it does work as expected. It seems to simply run both the targets, it may be possible to do it slightly differently, I'll see if I can make it cleaner. (But I'm not fantastic with makefiles)
There was a problem hiding this comment.
I thought I had an idea, but it turns out I already tried it and it didn't work, unfortunately I've yet to find a better approach. It seems this is a thing called Target-specific Variable Values. I know it works in gcc, but I don't know for clang or osx.
There was a problem hiding this comment.
https://www.gnu.org/software/make/manual/html_node/Multiple-Rules.html seems to state that it actually would not work as we expect:
There can only be one recipe to be executed for a file. If more than one rule gives a recipe for the same file, make uses the last one given and prints an error message.
Also see https://docs.oracle.com/cd/E19504-01/802-5880/make-24/index.html and https://www.gnu.org/software/make/manual/html_node/Double_002dColon.html#Double_002dColon.
I'd suggest to rename in terms of the original makefile libJyNI to _libJyNI everywhere. Then introduce a new libJyNI: _libJyNI _debug with also changing
debug: CFLAGS += -g
debug: all
to
_debug: CFLAGS += -g
debug: all _debug
We can do the same for the loader but maybe changes in the loader are so rare that it is sufficient to let it just build everything then.
|
Thanks for this work. I added two comments which I am not sure if they are issues. Maybe it's just questions or potential concerns. With these resolved I'll gladly merge this. |
|
I've also realised that the way It's set up, everything is always in debug mode... The only way to fix that appears to have yet more build targets. I'll also have to update the wiki to reflect the changes |
makefile
Outdated
| build-tests: | ||
| @echo 'building tests is not fully supported yet, this will either not work or try to install the demo extension as an actuall extension' | ||
| python ./DemoExtension/setup.py install | ||
| cd /scratch/calum/JyNI_summer/JyNI/DemoExtension && python ./setup.py build || echo "" && echo "Building tests failed" |
There was a problem hiding this comment.
opps, should be a ./DemoExtension, I'll fix it with the osx and clang changes
(also fixed build-tests to be a relative path)
|
I would also comment that these commits should probably all be squashed with a commit message that describes what changed rather than being split between so many commits. Something like: Added tests, debug mode and partial cleans/debug builds to the makefile (updated clang and osx to match) |
"Building tests failed" was printed whether they failed or not due to bad formatting.
|
What do you think about my suggestion in #33 (comment)? That would avoid multiple targets and https://www.gnu.org/software/make/manual/html_node/Multiple-Rules.html leaves me a bit puzzled about whether duplicate target is really the right thing. This might be implementation dependent behavior of make tools. (There is not only gnu make around, also cmake ,certainly even more). |
|
Its probably a good idea to do the libJyNI -> _libJyNI change so that it fits the direct caller wanting a debug build idea. I tried the suggestion, but it didn't seem to work, I think that the I think it is a separate thing with unhelpfully similar syntax, basically I don't know, but I would be tempted to leave it in (possibly with a warning comment) and not worry until someone runs make and it doesn't work. We could put some checks in that warned you if it was meant to add -g and didn't, but I'm not sure how to do that (I'm not a make wizard). |
Directly calling libJyNI(-Loader) gives the debug version, calling make all uses _libJyNI to give non-debug version
Debug and test modes have been built, and there are now builds and cleans for each of the separate eclipse projects, that way you can configure the builds properly in eclipse.
I've also updated the makefile.clan and makefile.osx but I'm not sure how to deal with the windows build.