Better Makefile target parsing by adamjstewart · Pull Request #8223 · spack/spack

adamjstewart · 2018-05-21T04:13:27Z

While working on the cryptopp package, I ran into a GNUmakefile that didn't have a ^check: or ^test: target, it had a ^test check: target. Apparently Makefiles can contain multiple targets that act as aliases. They can also have a space between the target and the colon. All of these things break our previous Makefile parsing logic.

I started playing around with regexes, but things quickly became complicated. I discovered an easier way to use make to query whether or not a specific target exists.

Pros

More reliable Makefile parsing.

Cons

This approach only works for GNU Make, it fails for BSD Make. I'm not sure how common BSD Make is (macOS and Linux both use GNU Make). The downside to this approach is that if someone runs spack install --test on a system that uses BSD Make, it will always think that a check target exists and run it even if it doesn't. The solution would either be to install GNU Make or to re-run spack install without --test if the test crashes.

P.S. I found a similar feature for Ninja and implemented that as well.

adamjstewart · 2018-05-21T04:14:35Z

lib/spack/spack/package.py

+                break
+        else:
            tty.msg('No Makefile found in the build directory')
            return


This block isn't actually needed anymore, it is only used in the final output message. Do you think it's still worth keeping?

What about something like:

if not any(os.path.exists(makefile) for makefile in ['GNUmakefile', 'Makefile', 'makefile']): tty.msg('No Makefile found in the build directory') return

?

I personally find this more readable, and you suggestion will need to be split onto multiple lines anyway. Plus, this results in makefile being set to the Makefile name, whereas I don't know if your suggestion does.

Fair enough. Readability is subjective.

davydden · 2018-05-21T05:39:57Z

The solution would either be to install GNU Make or to re-run spack install without --test if the test crashes.

or you can fallback to the old code with regex if make is not a GNU make.

adamjstewart · 2018-05-21T21:37:41Z

Is the BSD Make executable actually called make? When I install the bmake package, I get an executable called bmake. I don't have access to a BSD system...

citibeth · 2018-05-22T00:50:45Z

BSD make was the original, and its executable was called make. GNU Make was a clone, and its executable was also called make; or sometimes gmake.

I'm not against listing GNU make as a requirement; especially since it's already part of Linux and macOS, the two target platforms we run on.

adamjstewart · 2018-05-28T13:16:32Z

Ping @alalazo

@tgamblin may also be necessary to make an executive decision as to how important non-GNU Make support is. If we really want to be careful, I can implement @davydden's suggestion and fall back on regexes.

adamjstewart · 2018-06-12T13:25:07Z

Ping ping

scheibelp · 2018-06-12T19:01:25Z

lib/spack/spack/package.py

+        #
+        # https://www.gnu.org/software/make/manual/html_node/Running.html
+        #
+        # NOTE: This only works for GNU Make, not NetBSD Make.


When you say:

The downside to this approach is that if someone runs spack install --test on a system that uses BSD Make, it will always think that a check target exists and run it even if it doesn't.

How does this compare to the previous behavior for BSD make in Spack?

What is the exit status if you run this for BSD make? Can you distinguish a failing BSD make from a GNU make that is failing because the target doesn't exist?

Previously, we would grep the file for a ^check: target. It didn't matter if it was BSD or GNU Make, it would behave identically. The problem was that this regex isn't powerful enough to capture all possible check targets (see first paragraph of PR description).

The current behavior is that it is guaranteed to work perfectly for GNU Make but doesn't work at all for BSD Make.

With BSD Make, there is no separate exit status for "this target doesn't exist", it's the same exit status as "the target has not yet been built", so there is no way to distinguish by exit status alone. We could fall back on a regex if we need to support BSD Make, although I've never encountered a system that uses BSD Make.

adamjstewart · 2018-06-22T15:07:04Z

Ping @alalazo

alalazo

Concerning BSD Make: I wonder how many people have only that at their disposal. My guess is not a lot. In that case the impact of this PR wouldn't be that heavy in the sense that people impacted by a loss of functionality are those running:

$ spack install --run-tests ...

on those few systems.

alalazo · 2018-06-23T13:36:37Z

lib/spack/spack/package.py

+                break
+        else:
            tty.msg('No Makefile found in the build directory')
            return


What about something like:

if not any(os.path.exists(makefile) for makefile in ['GNUmakefile', 'Makefile', 'makefile']): tty.msg('No Makefile found in the build directory') return

?

adamjstewart · 2018-06-23T14:04:48Z

There are alternatives to this PR, or things we could try in the case of BSD Make:
https://unix.stackexchange.com/questions/230047/how-to-list-all-targets-in-make
https://gist.github.com/pvdb/777954

I would have to see how many of those flags work across all versions of Make though.

adamjstewart · 2018-07-12T14:45:25Z

Ping

scheibelp · 2018-07-14T02:08:16Z

We could fall back on a regex if we need to support BSD Make

On account of that I'm inclined to merge this. If I understand correctly this would mean that for GNU makefiles without a check target you'd end up doing the make check and the regex (that's not a problem IMO just making sure I understand how that update would work).

I'll get a chance to read through this again by the end of Tuesday 7/17.

adamjstewart · 2018-07-14T16:02:14Z

@scheibelp Here's a summary of the previous and current behavior:

Before

Identical behavior for GNU Make and BSD Make.

No false positives, many false negatives.

Ignored presence of GNUmakefile. Ignored situations like the following:

check test:
test check:
check :
foo check bar:

After

Different behavior for GNU Make and BSD Make.

No false positives or false negatives for GNU Make, 100% false positive rate for BSD Make.

I'm not sure how common non-GNU Make is. If you are concerned, we could add logic to fall back on regexes if non-GNU Make is detected. If anyone can think of a better regex than ^check:, let me know.

scheibelp · 2018-07-18T02:37:56Z

If I understand correctly this would mean that for GNU makefiles without a check target you'd end up doing the make check and the regex (that's not a problem IMO just making sure I understand how that update would work).

100% false positive rate for BSD Make.

Got it, I was incorrect in #8223 (comment): when you say "fall back to regex", we'd need to know that we are dealing with BSD make in the first place.

make -v reports "GNU Make" for me, which is satisfactory if it is dependable across GNU make versions, and if BSD make doesn't report something similar (it looks like -v doesn't retrieve version info at all for BSD make: https://www.freebsd.org/cgi/man.cgi?make). I think that works well enough that we could at least add that information to an error message if make(target) in _if_make_target_execute fails.

In other words I wanted to make sure we can figure out whether we are using BSD or GNU make if we need to (later, not in this PR), and based on the above I think we can. If you agree then I'm fine merging this as is (I'll get a chance to do that tomorrow).

adamjstewart · 2018-07-18T12:57:10Z

Yes, checking the output of make -v should be sufficient to determine whether we are using GNU Make if we need to.

scheibelp · 2018-07-18T18:11:17Z

Thanks!

#8223 replaced regex-based makefile target parsing with an invocation of "make -q". #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. #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.

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.

Better Makefile target parsing

b00ba35

adamjstewart added the makefile label May 21, 2018

adamjstewart requested a review from alalazo May 21, 2018 04:13

adamjstewart commented May 21, 2018

View reviewed changes

adamjstewart mentioned this pull request May 25, 2018

Add GDAL 2.3.0 #8263

Merged

scheibelp reviewed Jun 12, 2018

View reviewed changes

adamjstewart mentioned this pull request Jun 15, 2018

Actually allow parallel test execution under CMake. #8484

Merged

alalazo approved these changes Jun 23, 2018

View reviewed changes

adamjstewart mentioned this pull request Jun 28, 2018

[WIP] New package.py functions _has_make_target() and _has_ninja_target(). #8604

Closed

scheibelp merged commit a67139f into spack:develop Jul 18, 2018

adamjstewart deleted the features/make-check branch July 18, 2018 18:13

This was referenced Jul 22, 2018

Branch with the meson build-system #8467

Merged

flux: update flux-core package and add a flux-sched package #8818

Merged

adamjstewart added the tests General test capability(ies) label Jul 28, 2018

adamjstewart mentioned this pull request Jul 28, 2018

Even better Makefile target parsing #8819

Merged

Conversation

adamjstewart commented May 21, 2018

Pros

Cons

Uh oh!

adamjstewart May 21, 2018

Choose a reason for hiding this comment

Uh oh!

alalazo Jun 23, 2018

Choose a reason for hiding this comment

Uh oh!

adamjstewart Jun 23, 2018

Choose a reason for hiding this comment

Uh oh!

alalazo Jun 25, 2018

Choose a reason for hiding this comment

Uh oh!

davydden commented May 21, 2018

Uh oh!

adamjstewart commented May 21, 2018

Uh oh!

citibeth commented May 22, 2018

Uh oh!

adamjstewart commented May 28, 2018

Uh oh!

adamjstewart commented Jun 12, 2018

Uh oh!

scheibelp Jun 12, 2018

Choose a reason for hiding this comment

Uh oh!

adamjstewart Jun 12, 2018

Choose a reason for hiding this comment

Uh oh!

adamjstewart commented Jun 22, 2018

Uh oh!

alalazo left a comment

Choose a reason for hiding this comment

Uh oh!

alalazo Jun 23, 2018

Choose a reason for hiding this comment

Uh oh!

adamjstewart commented Jun 23, 2018

Uh oh!

adamjstewart commented Jul 12, 2018

Uh oh!

scheibelp commented Jul 14, 2018

Uh oh!

adamjstewart commented Jul 14, 2018

Before

After

Uh oh!

scheibelp commented Jul 18, 2018

Uh oh!

adamjstewart commented Jul 18, 2018

Uh oh!

scheibelp commented Jul 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants