A first pass at converting from CVS to git. #127

zhenya1007 · 2016-12-05T00:40:53Z

Just a quick pass over the Makefile, replacing usage of CVS commands
with (what I believe are) their approximate git equivalents.

My end goal is to have "make pkg" produce a fully-usable Emacs package. It isn't quite there yet: after installing ProofGeneral-20.tar with package-install-file, doing M-x locate-library proof-site complains that it cannot find proof-site, so something needs to be fixed (also byte-compilation that runs at the package installation time generates a lot of errors and warnings). Loading proof-site.el "by hand" does appear to "make things work."

Regardless of the issues with the package, I hope you can accept the conversion from CVS to git in Makefile.devel.

Just a quick pass over the Makefile, replacing usage of CVS commands with (what I believe are) their approximate git equivalents.

cpitclaudel · 2016-12-05T01:09:14Z

Makefile.devel

 ##         to VERSION, and edit $(DOWNLOADHTMLS)
 ##	   if VERSION matches PRERELEASE_TAG.
 ##
-tag:		
+tag:
 	@echo "*************************************************"
 	@echo " Tagging sources... (you must rerun if CVS source dirty)"


Leftover mention of CVS here :)

cpitclaudel · 2016-12-05T01:11:35Z

Thanks, this looks great! Having a package-able PG would be great.
I'll let @erikmd comment, since he's better versed than I am in PG-packaging-related matters.

One issue that you'll run into is that PG has subdirectories containing autoload comments; these are not picked up by package.el, so we need to be a bit smarter and put some autoloaded initialization code in a file at the root.

erikmd · 2016-12-05T23:50:54Z

Hi @cpitclaudel ! Thanks @zhenya1007 for submitting this. Indeed the PG Makefiles deserve some cleanup as there is no CVS around anymore. I won't have much time before the end of the week because of a deadline, but I'll try my best to review this asap.
Best regards.

zhenya1007 · 2016-12-06T00:01:47Z

Hi, @erikmd: no worries. I am sure we will all survive if this patch doesn't get reviewed until some time next week. :-) Thank you @cpitclaudel for the tip about autoloads trickiness: I have noticed that the ProofGeneral-autoloads.el as generated by package.el ends up being empty, and now I understand why.

cpitclaudel · 2016-12-06T00:42:03Z

@zhenya1007 On that topic, I think you'll find the discussion and PR at realgud/realgud#84 interesting.

cpitclaudel · 2016-12-06T00:42:15Z

Thanks again for working on this!

@cpitclaudel

This is essentially a variation on the approach suggested by @cpitclaudel. The idea is that, since `package-install*' will call `update-directory-autoloads' in the top-level directory of the package being installed exactly once at installation time, code to "bootstrap" autoloads in sub-directories can be put into a *.el file in the top directory of the package.

zhenya1007 · 2016-12-07T06:00:18Z

Ah, thank you that's a very nice approach. But I am not sure I understand why you want to generate the autoloads file, and then ship it. It seems to me, that, since the package-install-* function(s) will call update-directory-autoloads in the top-level directory of the package exactly once (at package installation time), you could have the code that generates the autoloads file in a "bootstrap" file at the top directory of your package. I will start a separate pull request to show exactly what I mean. Of course, it's entirely possible I am missing something.

cpitclaudel · 2016-12-07T06:06:59Z

I don't think you're missing anything :) It just seemed safer to pre-process the autoloads than to build them dynamically, but as long as we test the process appropriately install-time generation should work too.

I'll have time to review the other PR this week-end, I think.

cpitclaudel · 2016-12-07T06:07:04Z

Thanks again!

erikmd

Hello @zhenya1007, I have just reviewed your PR and inserted comments regarding some required changes.
To address them, can you please commit on your fork branch (zhenya1007:master) without doing any rebase nor push -f (so the link between comments and the code will be kept).
Thanks!

erikmd · 2016-12-26T21:40:09Z

Makefile.devel

 # anywhere for web pages/whatever).
-PRERELEASE_PREFIX=4\.4\.1~pre
-PRERELEASE_TAG=4.4.1~pre
+PRERELEASE_PREFIX=4\.4\.1-pre


Can you revert these two lines? (94-95)
I think it is not useful changing the naming convention for pre-release versions, and using a tilde (~pre), not a dash (-pre), is common practice in distributions such as Debian, see for example this page and that one

I certainly can: it's just text. :-) However, isn't this project ultimately producing an Emacs package? If so, I submit that the relevant guide lines with respect to version numbers are the guide lines for Emacs package version numbers. My reading of those guide lines does not lead me to believe that "~" will work "out of the box" as part of an Emacs package version string ("out of the box" = "without having the user customize version-separator and/or version-regexp-alist").

Hi @zhenya1007, thanks for your suggestion. Indeed, 4.4.1~pre won't work out of the box with function version-to-list. But on the other hand, this issue with the tilde is not that critical currently, as the suffix ~pre is not intended to be kept for PG releases: it was just added as a "SNAPSHOT" qualifier within the Git repo. However, according to the default value displayed in the documentation of (describe-variable 'version-regexp-alist), a -git suffix would actually be preferable for that usage...
Thus, for the moment (to ensure the monotonicity of the version number) I propose you revert that change back to 4.4.1~pre in the Makefile.devel file, and I shall apply this new suffix -git after the 4.5 version is released (namely, switching to something like 4.5.1-git).

Thank you for considering the issue. Your approach makes sense to me.

erikmd · 2016-12-26T21:50:55Z

Makefile.devel

-	@echo "***** CLEANING UP ALL NON-CVS FILES ****"
-#	rm -rf $(FILES_NONCVS)
+	@echo "***** CLEANING UP ALL NON-GIT FILES ****"
+	# git clean -fx


Deleting all non-tracked files can be done using git clean -d -x -f but this command is not useful as part of the Makefile (and currently it is commented-out).
I suggest you remove this section (i.e., from lines 248 to lines 256).

erikmd · 2016-12-26T22:18:04Z

Makefile.devel

 ChangeLog:	FORCE
-	$(RCS2LOG) -h "inf.ed.ac.uk" $(DEVELOPERS) -r -b -i 4 | sed 's|/home/proofgen/src/ProofGeneral/||g' > ChangeLog.prefix
+	git log --oneline --decorate > ChangeLog.prefix


The --decorate option is not very helpful and the use of a temporary file ChangeLog.prefix is not mandatory as there is no ChangeLog file in the Git repo.
I suggest replacing this line and the three subsequent ones with:
git log --pretty="format:%ad %s" --date=short > ChangeLog

erikmd · 2016-12-26T22:21:59Z

Makefile.devel

 	if [ -f ChangeLog ]; then mv ChangeLog ChangeLog.old; else echo > ChangeLog.old; fi
 	cat ChangeLog.prefix ChangeLog.old | sed 's|/disk/cvs/proofgen/ProofGeneral/||g' > ChangeLog
 	rm ChangeLog.prefix ChangeLog.old

 logupdate:  ChangeLog.gz
-	cvs commit -m"Updated" ChangeLog.gz
+	git add ChangeLog.gz


In line with my comment regarding the ChangeLog target, I suggest you remove the logupdate target and the two corresponding command lines.

erikmd · 2016-12-26T22:25:17Z

Makefile.devel

 ##         to VERSION, and edit $(DOWNLOADHTMLS)
 ##	   if VERSION matches PRERELEASE_TAG.
 ##
-tag:		
+tag:
 	@echo "*************************************************"
 	@echo " Tagging sources... (you must rerun if CVS source dirty)"


As pointed out by Clément, this comment should be updated: you can remove the sentence between parentheses "(you must rerun if CVS source dirty)".

erikmd · 2016-12-26T23:02:12Z

Makefile.devel

 	@echo "*************************************************"
 	@echo " Tagging sources... (you must rerun if CVS source dirty)"
 	@echo "*************************************************"
 # Update the sources, this is almost always what we want to do.
-	if [ -z "$(NOCVS)" ] && [ -n "`cvs -n -q update -Pd | grep '^[MC] '`" ]; then cvs update -Pd; exit 1; fi
+	if [ -z "$(NOCVS)" ]; then git status | grep '^Your branch is up-to-date with.*master' && git status | grep 'nothing to commit, working tree clean' || exit 1 fi


This line should be replaced with:
if [ -z "$(NOCVS)" ]; then git fetch origin && git log --exit-code master...origin/master || exit 1; fi

erikmd · 2016-12-26T23:13:25Z

Makefile.devel

@@ -334,26 +318,25 @@ tag:
 		rm $$f.old; \
 	   done) \
 	fi


This if-then-else (starting with if [ $(PRERELEASE_TAG) = $(VERSION) ]; then) should be commented-out as the PG website is now handled separately.

erikmd · 2016-12-26T23:17:20Z

Makefile.devel

@@ -334,26 +318,25 @@ tag:
 		rm $$f.old; \
 	   done) \
 	fi
-	if [ -z "$(NOCVS)" ]; then cvs commit -m"Set version tag for new release." generic/$(VERSIONFILE) etc/ProofGeneral.spec; (cd $(HTMLDIR); cvs commit -m"Set version tag for new release." $(DOWNLOADHTMLS)); fi
-	if [ -z "$(NOCVS)" ]; then cvs tag -cF "$(CVS_RELEASENAME)"; fi
+	if [ -z "$(NOCVS)" ]; then git add generic/$(VERSIONFILE) etc/ProofGeneral.spec && git commit -m"Set version tag for new release." ; git add $(HTMLDIR)/$(DOWNLOADHTMLS) && git commit -m"Set version tag for new release."; fi


You should remove the last command here, that is, remove ; git add $(HTMLDIR)/$(DOWNLOADHTMLS) && git commit -m"Set version tag for new release."

erikmd · 2016-12-26T23:19:23Z

Makefile.devel

-	if [ -z "$(NOCVS)" ]; then cvs commit -m"Set version tag for new release." generic/$(VERSIONFILE) etc/ProofGeneral.spec; (cd $(HTMLDIR); cvs commit -m"Set version tag for new release." $(DOWNLOADHTMLS)); fi
-	if [ -z "$(NOCVS)" ]; then cvs tag -cF "$(CVS_RELEASENAME)"; fi
+	if [ -z "$(NOCVS)" ]; then git add generic/$(VERSIONFILE) etc/ProofGeneral.spec && git commit -m"Set version tag for new release." ; git add $(HTMLDIR)/$(DOWNLOADHTMLS) && git commit -m"Set version tag for new release."; fi
+	if [ -z "$(NOCVS)" ]; then git tag -f  "$(GIT_RELEASENAME)"; fi


You should replace git tag -f "$(GIT_RELEASENAME)" with git tag -a "$(GIT_RELEASENAME)"

erikmd · 2016-12-26T23:25:55Z

Makefile.devel

@@ -362,12 +345,13 @@ cvsexport:
 	rm -rf $(DISTBUILDIR)
 	mkdir -p $(DISTBUILDIR)
 	if [ -z "$(NOCVS)" ]; then \
-           (cd $(DISTBUILDIR); \
-	    cvs -d $(CVSROOT) export -kv -r "${CVS_RELEASENAME}" -d ${RELEASENAME} ${CVSNAME}) \
+	    git worktree prune; \


git worktree is definitely overkill here.
Instead, you could replace these two lines with
git archive --format=tar --prefix=$(RELEASENAME)/ $(GIT_RELEASENAME) | (cd "$(DISTBUILDIR)" && tar xf -)
and remove the -rm -rf "$(DISTBUILDIR)/$(RELEASENAME)/.git" line below.

erikmd · 2016-12-26T23:53:42Z

Hi @zhenya1007,
Another detail:
as your commit 13b859d in this PR is almost the same as your commit 9e73368 in #128, I suggest that you also add a commit that reverts 13b859d in the fork branch related to this PR (zhenya1007:master), e.g. by doing git checkout master && git revert 13b859d
(Once the PR will be ready for merging, I'll do a squash-and-merge so 13b859d and the revert-commit won't appear anymore.)
Thank you again!
EMD

This reverts commit 9a7ac10.

Still need to resolve using "~" as a separator in the version number.

This reverts commit 13b859d.

zhenya1007 · 2016-12-27T21:35:17Z

Thank you for a thorough review. I have pushed to my fork a change that (as far as i can tell) addresses all of your comments with the exception of the version string comment: we'll probably need one more "round trip" to resolve that.

erikmd

Hi @zhenya1007,
Thanks for your commit! the only remaining details to address are at lines 94-95 and at line 304.
Best wishes for the New Year!

erikmd · 2016-12-31T13:17:37Z

Makefile.devel

 # anywhere for web pages/whatever).
-PRERELEASE_PREFIX=4\.4\.1~pre
-PRERELEASE_TAG=4.4.1~pre
+RERELEASE_PREFIX=4\.4\.1-pre


Hi @zhenya1007,
Regarding these two lines, in addition to my suggestion in #127 (comment), there is a missing P in "RERELEASE_PREFIX" ;-)

erikmd · 2016-12-31T13:18:11Z

Makefile.devel

+#		mv $$f $$f.old; \
+#		sed -e 's|ProofGeneral\([emacselc-]*\)-$(PRERELEASE_PREFIX)......|ProofGeneral\1-$(PRERELEASE_TAG)|g' $$f.old > $$f; \
+#		rm $$f.old; \
+#	   done) \
 	fi


This fi should be commented as well (the backslash in the previous line appears not to be sufficient).

A first pass at converting from CVS to git.

edfb351

Just a quick pass over the Makefile, replacing usage of CVS commands with (what I believe are) their approximate git equivalents.

cpitclaudel reviewed Dec 5, 2016

View reviewed changes

zhenya-1007 added 2 commits December 6, 2016 07:41

a draft for updating autoloads

13b859d

erikmd requested changes Dec 26, 2016

View reviewed changes

zhenya-1007 added 4 commits December 27, 2016 07:48

Revert "A possible approach to generating autoloads."

ab7eefa

This reverts commit 9a7ac10.

Address comments by @erikmd.

912c7f2

Still need to resolve using "~" as a separator in the version number.

Merge branch 'master' of github.com:zhenya1007/PG

7925f88

Revert "a draft for updating autoloads"

4226443

This reverts commit 13b859d.

erikmd requested changes Dec 31, 2016

View reviewed changes

Address the remaining comments by @erikmd.

dd9a878

erikmd approved these changes Dec 31, 2016

View reviewed changes

erikmd merged commit 8d405f3 into ProofGeneral:master Dec 31, 2016

erikmd mentioned this pull request Apr 10, 2017

XML code review #153

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A first pass at converting from CVS to git. #127

A first pass at converting from CVS to git. #127

zhenya1007 commented Dec 5, 2016

cpitclaudel Dec 5, 2016

cpitclaudel commented Dec 5, 2016

erikmd commented Dec 5, 2016

zhenya1007 commented Dec 6, 2016

cpitclaudel commented Dec 6, 2016

cpitclaudel commented Dec 6, 2016

zhenya1007 commented Dec 7, 2016

cpitclaudel commented Dec 7, 2016

cpitclaudel commented Dec 7, 2016

erikmd left a comment

erikmd Dec 26, 2016

zhenya1007 Dec 27, 2016 •

edited

erikmd Dec 31, 2016

zhenya1007 Dec 31, 2016

erikmd Dec 26, 2016

erikmd Dec 26, 2016

erikmd Dec 26, 2016

erikmd Dec 26, 2016

erikmd Dec 26, 2016

erikmd Dec 26, 2016

erikmd Dec 26, 2016

erikmd Dec 26, 2016

erikmd Dec 26, 2016

erikmd commented Dec 26, 2016

zhenya1007 commented Dec 27, 2016

erikmd left a comment

erikmd Dec 31, 2016

erikmd Dec 31, 2016

A first pass at converting from CVS to git. #127

A first pass at converting from CVS to git. #127

Conversation

zhenya1007 commented Dec 5, 2016

Choose a reason for hiding this comment

cpitclaudel commented Dec 5, 2016

erikmd commented Dec 5, 2016

zhenya1007 commented Dec 6, 2016

cpitclaudel commented Dec 6, 2016

cpitclaudel commented Dec 6, 2016

zhenya1007 commented Dec 7, 2016

cpitclaudel commented Dec 7, 2016

cpitclaudel commented Dec 7, 2016

erikmd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhenya1007 Dec 27, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikmd commented Dec 26, 2016

zhenya1007 commented Dec 27, 2016

erikmd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhenya1007 Dec 27, 2016 •

edited