Nicholas D Steeves <sten@debian.org> writes:
> 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?
>
There are about 3 years of newer commits since 1.1.0, and one of the
major changes is that it adds support of scala 3 syntax which is not yet
in a tagged release and would be good to have. Also seeing the last
commit is from the end of last year, I sense that upstream may becoming
a bit dormant for the time being, which is why I prefer to package the
latest snapshot instead of waiting for a stable release.
> Also, do you use this package?
>
Not on a regular basis, but I do use it a bit once in a while as I try
to learn a bit of new programming languages every few months.
>>>> * 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.
Oh yes it's generated using Cask following upstream practice. I just
include it as a patch as we don't use Cask for Debian packaging.
> 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.
Personally I prefer not to patch upstream source if not necessary,
otherwise we end up carrying the patch for the foreseeable future.
Though arguably in this case the scala-mode-pkg.el file needs to be
generated/patched which requires I use Cask locally, so it's more or
less the same effort.
And then I just realized: why not just host the scala-mode-pkg.el file
and substitute the version so that we don't need to update it manually
on each update? This is now implemented at [1].
> 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"?
Well I didn't choose it (if I did I'd use 1.1.0 :) This is the version
specified in scala-mode.el[2].
> 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)?
>
I tried install it from stable.melpa.org and yeah it's being
installed as scala-mode-0.23 even if it's registered as version 1.1.0
there[3]. So it's consistent in a sense :P
Anyway, I have just made a pull request to update this upstream[4] so
hopefully the versioning will be sane.
>>>> * 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.
>
Ack, and good suggestion! I have slightly extended the entry with the
rationale[5]. PTAL.
>>>> * 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.
>
Ack.
>>>> * 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!".
>
Ack.
On a related topic, I used to change the section from "lisp" to
"editors" following lintian suggestions, but it seems you and Sean are
not very keen on this. I'll try to start a thread on the mailing list
for suggestions.
>>>> * 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).
>
I meant to say "gbp buildpackage" which uses this field to specify the
compression type. Sorry for the confusion.
And you are right, "git deborig" will use xz with "3.0 (quilt)"
regardless of these options.
>> 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?
Yes, I meant "gbp" in this context. I think adding the options makes it
consistent with "git deborig", otherwise we may get "more than one orig
tarball is available" error (".tar.gz" from "gbp buildpackage" and
".tar.xz" from "git deborig").
> 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
>
This is because I didn't push the tag to the repo yet. Now this is
done.
> 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.
>
In this specific case as I'm targeting a snapshot so there is no
upstream tarball, so using xz provides the benefits of a smaller source
tarball that saves space for the Debian source repository.
For the aspect of the consistency with upstream release tarball, I'm
trying to understand how to make this work. I believe "gbp import-orig
--uscan" should grab the upstream release tarball which follows your
suggestion. However IIUC the repository is configure using the
dgit-maint-merge workflow that uses upstream branch to target upstream
repo and hence uses the tagged version to generate upstream tarball,
which I believe is incompatible with "gbp import-orig --uscan" approach
which directly import the release tarball without the git history. I
wonder whether there is a way for "git deborig" or "gbp buildpackage" to
grab upstream tarball automatically? I'm guessing not, so someone has
to do it manually, but please let me know if there is a way.
> Regards,
> Nicholas
>
[1] https://salsa.debian.org/emacsen-team/scala-mode-el/-/commit/fcb6d233254a2551957a71e37610b6cdd73824c4
[2] https://github.com/hvesalai/emacs-scala-mode/blob/master/scala-mode.el#L7
[3] https://stable.melpa.org/#/scala-mode
[4] https://github.com/hvesalai/emacs-scala-mode/pull/183
[5] https://salsa.debian.org/emacsen-team/scala-mode-el/-/commit/7a7947b25fa9a2e9352927b15444443b19e40f72
--
Xiyue Deng
Attachment:
signature.asc
Description: PGP signature