Even better Makefile target parsing#8819
Conversation
I gave it a whirl. This is the output I could see: I'm digging deeper now. |
|
Output from manually running it: |
|
Just a heads up, it appears that |
|
It seems that most of the trouble with flux-core/sched's Makefile's comes from the subdirectories and the recursive nature of the calls. Potentially stupid idea alert: what if we just use grep on the top-level Makefile? |
|
See #8223 for why grep wasn't sufficient. If we can come up with a better regex, that may be the last resort. |
|
@SteVwonder Can you try the latest commit? That should fix the problem you were seeing. I still don't like the fact that it errors out though. We need to decide how worrying the potential for check:
check :
test check:
check test:
foo bar check baz:but not: installcheck:
checkinstall: |
|
@adamjstewart: your latest commit seems to work: FWIW, Another potentially terrible idea is to actually parse the Makefile using pyparsing or similar. |
|
I might try that regex. I need to find some documentation on allowed target names (I think they can contain uppercase and dashes) but that might be just what I'm looking for. Avoiding accidentally running the Makefile or dealing with different versions of Make sounds nice. |
adamjstewart
changed the title
|
It looks like targets can contain:
We could handle a lot of that via the regex character class The |
|
@SteVwonder So far your regex is up to: There are two problems with this:
check: recursive-check
recursive-check:
the actual build instructionsSo we need to drop the
check := ${test}Does this seem sufficient? I wouldn't think this would match a line ending with a colon, but regex101.com seems to think otherwise. P.S. This is why I gave up last time. So many corner cases... If we really want to support the variable expansion stuff, we should just punt and stick with |
adamjstewart
changed the title
|
@adamjstewart: I'm leaning towards sticking with If you want to go with the regex, I can take another look at what you have so far. |
adamjstewart
force-pushed
the
fixes/make-check
branch
from
29a533b to
2f789f7
Compare
adamjstewart
changed the title
| @@ -0,0 +1,2 @@ | |||
| .ninja_deps | |||
| .ninja_log | |||
There was a problem hiding this comment.
Running ninja check generates these files. Should I try to clean them up? How do I do that in pytest?
adamjstewart
changed the title
|
Okay, this should be ready to merge now. I added unit tests for every conceivable Let me know if you want me to clean up those |
|
Are you still tweaking this? It looked good to me so I was going to merge it |
|
I'm done, I was just trying to clean up those pesky Ninja files. If the last commit looks good to you go ahead and merge. |
|
It looks good. However, I'm writing a commit message and I'm curious if you added 0222d1e (adding 'all' to the |
|
I added it only to be more explicit. The behavior is identical with or without |
|
It appears that a couple of the |
|
No idea. |
|
OK - I have added an issue on 3.7 test failures at #9028. For now, we allow those to fail so I'll merge this. |
|
@adamjstewart @scheibelp: is |
|
Develop seems to be passing? It only fails the Python 3.7 test. |
Imo, any test failing on Python 3.7 should be considered a "real" failure. In #8778 I added: allow_failures:
- dist: xenialjust because the way to get a worker on Travis with |
spack#8223 replaced regex-based makefile target parsing with an invocation of "make -q". spack#8818 discovered that "make -q" can result in an error for some packages. Also, the "make -q" strategy relied on interpreting the error code, which only worked for GNU Make and not BSD Make (which was deemed acceptable at the time). As an added bonus, this implementation ignores the exit code and instead parses STDERR for any indications that the target does not exist; this works for both GNU Make and BSD Make. spack#8223 also updated ninja target detection to use "ninja -t targets". This does not change that behavior but makes it more-explicit with "ninja -t targets all" This also adds tests for detection of "make" and "ninja" targets.
5 participants
This is a continuation of #8223.
In #8818, @SteVwonder discovered a bug in my implementation. For reasons I still don't fully understand,
make -qdoes not work as documented. This can easily be reproduced with thelibsigsegvpackage, which does not have any dependencies. Outside of Spack, we can see thatmake checkworks fine, butmake -q checkcrashes:The error code for
make -qwas also unreliable, it only worked for GNU Make, not BSD Make.The new implementation ignores the exit code and instead parses STDERR for any indications that the target does not exist. This works for both GNU Make and BSD Make.
I switched from
make -qtomake -n. It works similarly tomake -q(it doesn't actually run anything, just prints what would be run). The only difference is thatmake -qdoesn't print an error message when using BSD Make, butmake -ndoes.@SteVwonder would you mind testing this with #8818?
Closes #8604