openshmem-org / specification

OpenSHMEM Application Programming Interface
http://www.openshmem.org
50 stars 32 forks source link

Fix `make clean` #381

Closed BryantLam closed 4 years ago

BryantLam commented 4 years ago

make clean never worked correctly for me. The braces expansion does not work. This fixes it.

Tested only with:

jdinan commented 4 years ago

I tried with BSD make, just for fun. Current master works fine, make clean on this branch results in:

$ bsdmake clean
chappage.txt
chappage.txt:No such file or directory
*** Error code 1

Stop in /Users/jdinan/Documents/Specifications/OpenSHMEM/specification.

@BryantLam What make are you using (and which shell)?

tonycurtis commented 4 years ago

On Feb 27, 2020, at 4:35 PM, James Dinan notifications@github.com wrote:

I tried with BSD make, just for fun. Current master works fine, make clean on this branch results in:

$ bsdmake clean chappage.txt chappage.txt:No such file or directory *** Error code 1

Stop in /Users/jdinan/Documents/Specifications/OpenSHMEM/specification. @BryantLam https://github.com/BryantLam What make are you using?

Looks like BSD’s make doesn’t see an RM macro.

== all: @ echo $(RM)

freebsd12$ gmake rm -f

freebsd12$ make

$

tony

jdinan commented 4 years ago

Even if I replace $(RM) with rm -f, it's still broken:

$ bsdmake clean
rm -f  chappage.txt

@tonycurtis Do you build the spec on BSD?

tonycurtis commented 4 years ago

On Feb 27, 2020, at 4:58 PM, James Dinan notifications@github.com wrote:

Even if I replace $(RM) with rm -f, it's still broken:

$ bsdmake clean rm -f chappage.txt @tonycurtis https://github.com/tonycurtis Do you build the spec on BSD?

Just did so.

$ uname -a FreeBSD diablo 12.1-RELEASE-p2 FreeBSD 12.1-RELEASE-p2 GENERIC amd64 $ make … …

simple group (level 1) entered at line 32 ({)

bottom level

(see the transcript file for additional information) !pdfTeX error: pdflatex (file 8r.enc): cannot open encoding file for reading ==> Fatal error occurred, no output PDF file produced! *** Error code 1

Stop.

Not sure what I need.

However, I touch’ed chappage.txt and “make clean” doesn’t complain, deletes chappage.txt, but it doesn’t delete the .out/.idx etc. files.

Ah! It’s the Bourne shell on BSD. Doesn’t understand the {} notation, treats it literally.

Tony

PS The “out” suffix is listed twice.

BryantLam commented 4 years ago

I updated my original post with my config: gmake and bash; typical Linux setup.

Ah! It’s the Bourne shell on BSD. Doesn’t understand the {} notation, treats it literally.

This was the problem I was seeing as well. I couldn't figure out why it wouldn't expand {} but I didn't dig too deep into it.

tonycurtis commented 4 years ago

On Feb 27, 2020, at 8:02 PM, Bryant C. Lam notifications@github.com wrote:

I updated my original post with my config: gmake and bash; typical Linux setup.

Ah! It’s the Bourne shell on BSD. Doesn’t understand the {} notation, treats it literally.

This was the problem I was seeing as well. I couldn't figure out why it wouldn't expand {} but I didn't dig too deep into it.

Now building fully on FreeBSD 12.

Apart from the sh/{} issue, I am getting chappage.txt in the “content” directory, not the top-level that the rm-f expects.

Is chappage.txt supposed to be in the repo if make-clean zaps it?

Tony

jdinan commented 4 years ago

@tonycurtis Is the updated Makefile working for you verbatim, or did you need to make changes?

tonycurtis commented 4 years ago

On Feb 28, 2020, at 10:59 AM, James Dinan notifications@github.com wrote:

@tonycurtis https://github.com/tonycurtis Is the updated Makefile working for you verbatim, or did you need to make changes?

FreeBSD make doesn’t like the $(RM) and its Bourne shell doesn’t grok the {} expansion. So I have the current master HEAD:

.PHONY: clean clean: rm -f ${TARGET}.{log,aux,ps,dvi,bbl,blg,log,idx,ind,ilg,out,toc,pdf,out} chappage.txt

Which runs OK but doesn’t actually do anything ({} doesn’t expand, and chappage.txt in content subdirectory).

With the RM macro:

$ make clean main_spec.{log,aux,ps,dvi,bbl,blg,log,idx,ind,ilg,out,toc,pdf,out} chappage.txt /bin/sh: main_spec.{log,aux,ps,dvi,bbl,blg,log,idx,ind,ilg,out,toc,pdf,out}: not found *** Error code 127

Stop.

There’s a stray blank line at EOF too in Makefile.

Tony

tonycurtis commented 4 years ago

On Feb 28, 2020, at 11:06 AM, Tony Curtis anthony.curtis@stonybrook.edu wrote:

On Feb 28, 2020, at 10:59 AM, James Dinan <notifications@github.com mailto:notifications@github.com> wrote:

@tonycurtis https://github.com/tonycurtis Is the updated Makefile working for you verbatim, or did you need to make changes?

FreeBSD make doesn’t like the $(RM) and its Bourne shell doesn’t grok the {} expansion. So I have the current master HEAD:

.PHONY: clean clean: rm -f ${TARGET}.{log,aux,ps,dvi,bbl,blg,log,idx,ind,ilg,out,toc,pdf,out} chappage.txt

Which runs OK but doesn’t actually do anything ({} doesn’t expand, and chappage.txt in content subdirectory).

How about this?

RM ?= rm -f

EXTS = log aux ps dvi bbl blg log idx ind ilg out toc pdf out

.PHONY: clean clean: for e in $(EXTS); do $(RM) ${TARGET}.$$e; done $(RM) content/chappage.txt

jdinan commented 4 years ago

Wouldn't this be dependent upon the user running a shell that understands that for loop? I think the most portable solution is to list the complete filenames in a make variable. Unfortunately, portable make programming is extremely limited. But also, there are good tools specifically for building LaTeX documents.

tonycurtis commented 4 years ago

On Feb 28, 2020, at 2:25 PM, James Dinan notifications@github.com wrote:

Wouldn't this be dependent upon the user running a shell that understands that for loop? I think the most portable solution is to list the complete filenames in a make variable. Unfortunately, portable make programming is extremely limited. But also, there are good tools specifically for building LaTeX documents.

I suspect all Bourne shells around today support “for”. Even busybox’s ash. Just listing them all explicitly is the “simplest” portable way as you say.

Tony

jdinan commented 4 years ago

On a committee like this one, there just has to be a csh contrarian. :neckbeard:

tonycurtis commented 4 years ago

On Feb 28, 2020, at 4:27 PM, James Dinan notifications@github.com wrote:

On a committee like this one, there just has to be a csh contrarian.

Isn’t that what the “c” stands for? :)

Just list the filenames explicitly, keep it simple.

Tony

BryantLam commented 4 years ago

Personally, I like Tony's solution the best and would advocate implementing it because:

Side conversation: Tony, your solution has a misc change where you're now removing content/chappage.txt instead of the top-level chappage.txt. I don't know what chappage.txt actually does or how it's generated. I suspect that it being currently committed to the repo and not .gitignore'd is also a mistake.

BryantLam commented 4 years ago

The current Makefile on master doesn't clean properly because the braces expansion isn't POSIX sh compliant and, at least for GNU Make, rules are apparently executed with sh.

S := $(shell echo $$SHELL)
echo-shell:
        @echo $(S)

echo-rule:
        @echo $(SHELL)
$ make echo-shell
/bin/bash

$ make echo-rule
/bin/sh
tonycurtis commented 4 years ago

On Mar 10, 2020, at 11:33 PM, Bryant C. Lam notifications@github.com wrote:

Personally, I like Tony's solution https://github.com/openshmem-org/specification/pull/381#issuecomment-592648799 the best and would advocate implementing it because:

The for loop is POSIX sh compliant https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html (search for "The for Loop"). I dislike unnecessary duplication Side conversation: Tony, your solution has a misc change where you're now removing content/chappage.txt instead of the top-level chappage.txt. I don't know what chappage.txt actually does or how it's generated. I suspect that it being currently committed to the repo and not .gitignore'd is also a mistake.

There was never a chappage.txt in the top-level directory, only in the content/ subdir.

[tc@diablo /usr/home/tc]$ git clone git@github.com:openshmem-org/specification.git [tc@diablo /usr/home/tc]$ cd specification/

[tc@diablo /usr/home/tc/specification]$ make … … [tc@diablo /usr/home/tc/specification]$ ls content main_spec.aux main_spec.ind main_spec.pdf Makefile example_code main_spec.idx main_spec.log main_spec.tex README.md figures main_spec.ilg main_spec.out main_spec.toc utils

[tc@diablo /usr/home/tc/specification]$ ls -C content/ | head atomics_intro.tex shmem_global_exit.tex backmatter.tex shmem_iget.tex chappage.txt shmem_info_get_name.tex collective_intro.tex shmem_info_get_version.tex coverpage.tex shmem_init_thread.tex environment_variables.tex shmem_init.tex execution_model.tex shmem_iput.tex frontmatter.tex shmem_lock.tex interoperability.tex shmem_malloc_hints.tex language_bindings_and_conformance.tex shmem_malloc.tex

Deleting chappage.txt seems to have no effect on a subsequent make and it isn’t re-created.

Tony

jdinan commented 4 years ago

Is chappage.txt generated by your builds? I don't see it being generated when building the document on my Mac. If not, perhaps we should just remove the file and strike all mention of it from the repository?

tonycurtis commented 4 years ago

On Mar 11, 2020, at 1:53 PM, James Dinan notifications@github.com wrote:

Is chappage.txt generated by your builds? I don't see it being generated when building the document on my Mac.

Nope. It’s there in the clone, though. Only referenced in .gitignore and Makefile.

Tony

BryantLam commented 4 years ago

In that case, https://github.com/openshmem-org/specification/pull/381/commits/2000a9b861999726ab13ec89ba1f78e5a0110f02 simply removes all traces of chappage.txt since we don't seem to know where it is generated from.

https://github.com/openshmem-org/specification/pull/381/commits/baecb64f46217e3dc154e4a40db1de6540b39b32 does the same thing with the bibtex files and the other output formats (dvi, ps) since those aren't created anymore either.

jdinan commented 4 years ago

@tonycurtis Please review

jdinan commented 4 years ago

@BryantLam Should be ok if you want to merge master to your section branch to pull in this fix.