Xiyue Deng <manphiz@gmail.com> writes: > Nicholas D Steeves <sten@debian.org> writes: > > >> Have you asked upstream to tag a release? >> > > Not before your review but done by now at [1] Thank you. You may have heard that Debian is a distribution that privileges the stable release model... When the human maintainer of a Debian package tracks stable releases, why is importing a snapshot justified? Also, do you use this package? >>> * Override clean rules in d/rules to fix building. (Closes: >>> #1052917) >> >> I believe you already know that >> >> override_dh_auto_clean: >> /bin/true >> >> is an incorrect approach. >> > > Indeed it was not ideal. Upstream depends on Cask to generated the > scala-mode-pkg.el file that is used in the clean target to get the name > of the generated tarball, and indeed using this lazy approach is > incorrect. I've now included the generated pkg file through a patch to > make this work in [2]. Consistency is essential between an explanation (in a comment or changelog) and the work that was done. Statically defining package metadata is fine, but in this case you can't claim that you're generating the pkg.el file. Either make the changelog and patch description consistent with what is actually happening, or change the implementation so that something is actually generated (there are multiple approaches here). I think I tend to use makefile substvars for this. Do you see what will happen when the package is updated to 1.1.1 or newer? Also, why did you choose to set the version to "0.23" rather than "1.1.0"? Did you verify that elpa package version is consistent with the upstream version of the Debian package in bin:elpa-scala-mode that is consumed by users (the binary package)? >>> * Modernize d/watch using special substitute strings. >> >> Ok, but why? >> > > I believe this provides a more robust way of detecting tags and should > be an encouraged practices. From my own experience, when I find a > d/watch file that doesn't work I may search for other packages to learn > from existing practices, and some may not work well as different > upstream may follow different conventions. The substitute strings use a > more robust and tested regexp that works most of the time, and promoting > its use may save people's time instead of working on an ad-hoc regexp. Sounds good! This is the kind of rationale that should be in the changelog, so please add it there :) From now on, read your changelog and patche desriptions, and imagine I'm asking you "ok, but why" for each point. Yes, rarely something is self-evident and/or an implementation detail, but most of the time you should say a few words explaining "why"--particularly when you want to find a sponsor quickly. My expectation is that you get better at this with each review, and that you will apply everything you learned to all pending sponsorship requests in addition to future ones. >>> * Add more metadata in d/upstream/metadata. >> >> https://github.com/hvesalai/emacs-scala-mode/commits/master >> >> is a git history log, not a changelog nor release notes. >> > > I thought the git history log may be considered an alternative form of a > Changelog. Looks like I was wrong except for projects that requires the > same format across changelog/git history/release notes. I've dropped > that line in [3]. Thank you. Re: "projects that requires the same format across changelog/git history/release notes": Changelogs, NEWS files, and release notes are three (or arguably two) distinct types of documentation that are also distinct from VCS history. This isn't a superficial formatting or style thing, because they have different audiences and purposes. I think that the kind of changelog that you're probably thinking of it when upstream takes git's shortlog history, puts it in a file, and edits it so that it makes sense. >>> * Update year and Upstream-Contact and add myself in d/copyright. >> >> Why did you add yourself? >> https://en.wikipedia.org/wiki/Threshold_of_originality >> >> I'm happy to support your claim, but you'll need to work for it in more >> than a "sweat of the brow"/mechanical sense. >> > > To be honest, the only reason I did this is to suppress the > "update-debian-copyright" lintian warning which is actually > experimental. I believe what I did was in the same nature as Sławomir > did in 2020 though admittedly not to the same extent, so I've reverted > this part in [4]. Cool. Yeah, lintian has these tags: error, warning, info, pedantic, experimental. Which ones do you think are suggestions, and which one[s] require a mandatory fix? Note that suggestions are occasionally highly opinionated and conflict with team policy. As ever, it's not sufficient to simply react to lintian: ie "lintian made me do it!". >>> * Use xz compression in d/gbp.conf. >> >> Why is this useful when it has been the default since gbp 0.9.15? >> > > I'm pretty sure that if I don't add this "git deborig" will create the > tarball using gzip instead. Really? It looks to me like "git deborig" doesn't have anything to do with gbp. (see the man page as well as the source for git-deborig:L181). > And it looks like the commit from 0.9.15 just changed the value in the > comment[5]. Please let me know if there is any other option that I > missed that makes it use xz. Sorry, I don't follow. What "it" in "makes it use xz" are you talking about? Gbp? Meanwhile, compression type isn't the problem: gbp:info: Tarballs 'scala-mode-el_1.1.0+git20221025.5d7cf21.orig.tar.gz' not found at '/home/sten/devel/tarballs/' gbp:error: v1.1.0+git20221025.5d7cf21 is not a valid treeish and at the same time, the proposed switch to xz is also arguably a regression, because the actual upstream release tarballs of scala-mode are currently being used: $ apt source scala-mode-el $ md5sum scala-mode-el_1.1.0.orig.tar.gz 615b1f38ed083137fee99fa83fe452a1 scala-mode-el_1.1.0.orig.tar.gz # Download release tarball from github manually, or use uscan $ md5sum ~/Downloads/emacs-scala-mode-1.1.0.tar.gz 615b1f38ed083137fee99fa83fe452a1 /home/sten/Downloads/emacs-scala-mode-1.1.0.tar.gz This indicates that the human maintainer doesn't use "git deborig". Being able to reproduce the imports of official upstream tarballs is important to a number of developers, and some workflows depend on this assumption, so please don't break it. Regards, Nicholas
Attachment:
signature.asc
Description: PGP signature