- Notifications
You must be signed in to change notification settings - Fork1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[minor] harden config/Makefile against '#' in PREFIX#1214
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
With the two changes I suggest, I get an identical config/Makefile with the merge base.
configure Outdated
| echo "PROGRAMS_MAN_SECTION=$programs_man_section" >> Makefile | ||
| echo "LIBRARIES_MAN_SECTION=$libraries_man_section" >> Makefile | ||
| config "PROGRAMS_MAN_SECTION=$programs_man_section" | ||
| config "LIBRARIES_MAN_SECTION=$libraries_man_section" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should be:
config PROGRAMS_MAN_SECTION "$programs_man_section"config LIBRARIES_MAN_SECTION "$libraries_man_section"configure Outdated
| # $(PREFIX) is also injected in C code where this escape is invalid | ||
| # -- see the definition of the OCAML_STDLIB_DIR macro below. | ||
| echo $1=$2 | sed 's/\#/\\#/g' >> Makefile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The$1=$2 needs double-quoting or spaces are being trimmed. It's irrelevant in most cases, but it removes the space at the end ofPACKLD which, amongst other things, I think is why the lib-dynlink-native test is failing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
So I tried to useecho $1=\"$2\" here but I don't like the fact that it makes the result very different from the previousconfig/Makefile where variables are not quoted. In the next iteration (I'll push now), I used"$partialld -o\\ \$(EMPTY)" instead as some other variables were already doing, does that sound reasonable to you?
If I diff the config/Makefile before and after the change, the differences are:
CPPFLAGShad a trailing space that is now gone (I think this is correct, it comes from the space before the empty variableinternal_cppflags)BYTECCLIBShas 18 trailing spaces that are now gone (that again seems correct)NATIVECCLIBShad two spaces instead of one in the middle- and
PACKLDthat needs a fix as you pointed out
Sorry, I should have made this comparison earlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I meant"$1=$2" which results in an identical Makefile for me?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
aah, sorry. Should I keep the $(EMPTY) trick for extra safety?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
So I used"$1=$2" and indeed the result are identical, which is good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes - the$(EMPTY) trick is much better than what was there before. What happened was that trimming the space meant you escaped the newline, so it read the following line into PACKLD (creating garbage). If the space after the-o is critical (it is for some C compilers), then it should be obviously critical and$(EMPTY) is a good way of doing that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
dra@bean:~/ocaml$ ./ocamlc.opt -config | fgrep native_packnative_pack_linker: ld -r -o IFLEXDIR=There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
NATIVECCLIBS gets two spaces because$mathlib is empty (comes via$cclibs);BYTECCLIBS gets 18 spaces because of the "bad" line continuation. If it's changed to:
echo"BYTECCLIBS=$cclibs$dllib$curseslibs$pthread_link" \"$instrumented_runtime_libs">> Makefile
then it should work, however, none of them causes a problem as all those spaces get compacted when make loads config/Makefile so I'm not sure I'd change any of it (as much as I like perfectly generated files!)
I like this change, but I wonder whether@shindere would prefer this kind of alteration post-autoconf (or considered with autoconf?) |
6b6c8f4 toe90a72aCompareAll checks now pass. Given that the change is not very invasive (it does not affect the structure of the code in any way), and that it fixes a bug for me (git branch names with a P.S.: I forgot to update the Changes entry, will do. |
e90a72a tobfb25a2Compare(@dra27 do you mean to "approve" the PR? We are now in required-review mode. ) |
I'm happy to approve it, but I would like Sébastien to have a chance to comment! |
David Allsopp (2017/07/07 09:53 +0000): I like this change, but I wonder whether@shindere would prefer this kind of alteration post-autoconf (or considered with autoconf?) @dra27: are you talking about the "config" function?In that case: I think it will simply go away with au"toconf. What willhappen is that there will be a config/Makefile.in (or Makefile.config.inin the toplevel source tree) with lines likePROGRAMS_MAN_SECTION="@@programs_man_section@@"Running configure a similarly named file (without the .in suffix) wherethe @@variables@@ are replaced by the values computed by configure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM and@shindere has not expressed any opposition
configure Outdated
| # $(PREFIX) is also injected in C code where this escape is invalid | ||
| # -- see the definition of the OCAML_STDLIB_DIR macro below. | ||
| echo "$1=$2" | sed 's/\#/\\#/g' >> Makefile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You don't need the first\.
bfb25a2 toba85206CompareThanks, rebased. Someone (or myself) can merge if the CI passes again. |
ba85206 to890c8bbCompareThe opam-compiler-conf script will generate an opam switch name(and thus a directory name) from the name of the current gitbranch. Branches named 'PR#1234-foo-bar' would have the ./configurescript generate a config/Makefile with the lines PREFIX=~/.opam/4.06.0+local-git-PR#1234-foo-bar BINDIR=$(PREFIX)/bin BYTERUN=$(BINDIR)/ocamlrun LIBDIR=$(PREFIX)/lib/ocamlThe '#' in the first line parses as the start of a comment, sothe second part is ignored and the build system would then installin ~/.opam/4.06.0+local-git-PR instead.After this change, config/Makefile starts with: # generated by ./configure --prefix ~/.opam/4.06.0+local-git-PR#1234-foo-bar CONFIGURE_ARGS=--prefix ~/.opam/4.06.0+local-git-PR\ocaml#1234-foo-bar PREFIX=~/.opam/4.06.0+local-git-PR\ocaml#1234-foo-bar
The opam-compiler-conf script will generate an opam switch name
(and thus a directory name) from the name of the current git
branch. Branches named 'PR#1234-foo-bar' would have the ./configure
script generate a config/Makefile with the lines
The '#' in the first line parses as the start of a comment, so
the second part is ignored and the build system would then install
in ~/.opam/4.06.0+local-git-PR instead.
After this change, config/Makefile starts with: