Code review comment for lp:~dobey/libubuntuone/srcdir-signing

Revision history for this message
Loïc Minier (lool) wrote :

That seems fine; some minor comments:
 * you might want to call sn under LC_ALL=C in case the output is localized at some point -- or test exit code if that's an option, not sure it reports this in the exit code though
 * you might want to send errors to stderr
 * instead of sn | sed + test, you could just grep? e.g.:
if LC_ALL=C $(MONO_SN) -T $< | grep -q '^Public Key Token: '; then \
   echo "Assembly was not properly signed." >&2; \
   exit 1; \
fi

 * I guess it's a personal taste question, but I think sed s/[@]FOO[@]/bar/ is slightly more common in Makefile.am than s/\@FOO\@/bar/

 * the Makefile dependency is weird:
+AssemblyInfo.cs: $(srcdir)/AssemblyInfo.cs.in $(srcdir)/mono.snk Makefile
 I don't think any other generated file depends on Makefile

 * I'm not actually sure why AssemblyInfo.cs depends on mono.snk; I think only the dll needs to do that

 * thanks to VPATH, I think you can actually just depend on the plain simple filenames:
AssemblyInfo.cs: AssemblyInfo.cs.in

« Back to merge proposal