Nicholas D Steeves <sten@debian.org> writes: > Xiyue Deng <manphiz@gmail.com> writes: > >> Nicholas D Steeves <sten@debian.org> writes: >>> Xiyue Deng <manphiz@gmail.com> writes: >>>> Nicholas D Steeves <sten@debian.org> writes: >>>> >> >> 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. > > Ok, you've convinced me :) Convey this information in your changelog > (that's what it's for), because the human maintainer (and any interested > users) of this package deserves to know why you made this change. Indeed, updated the changelog with this[2]. > >> 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. > > This can also mean that we run the risk of becoming defacto upstream if > they quit at this point, but that said, I agree it's a good cut point as > well as the right thing to do. > >>> 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. > > Thanks for confirming! > > [snip] > >> 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]. > > Substvars make sense ;) > > Also, neat use of a makefile target called from within the dh sequence. > > Are you sure dh_auto_clean is the right place for this override? Skim > its man page, as well as the one for dh_clean before replying. Also, > whenever one overrides something in rules, one needs to document this in > the changelog. I think dh_auto_clean is the right place, because the build failure is because that the clean target requires the existence of scala-mode-pkg.el, which is generated by Cask. As we don't have Cask, we need to provide this before dh_auto_clean runs. > > Please use "cp -a" so timestamps between builds will be reproducible. Done. > What is the rationale for CURDIR? From what I can tell it isn't needed > and should be dropped. I can't find a packaging suggestion on $(CURDIR), but it looks like it also works without it, so I dropped it[3]. > >>> 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]. > > I like your choice, and so what if upstream has that! Is it correct? > >>> 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 > > Oh my! Sorry for the convoluted sentence I wrote, and I'm impressed > that you were able to make sense of it. Yes, stable.melpa.org publishes > a scala-mode version 1.1.0 elpa package, which contains a scala-mode.el > file with "Package-Version: 0.23", and it also contains a > scala-mode-pkg.el file that overrides the Package-Version to `1.1.0`. > It is because of this pkg.el file that elpa/scala-mode-1.1.0 subdir. > > Meanwhile our elpa-scala-mode 1.1.0-* all declare 0.23, and install to a > scala-mode-0.23 subdir. Thank you for your kind optimism, that's very > gracious. > > Your work reveals that I missed this issue when reviewing 1.1.0-1, > which I appreciate having pointed out. Lets fix it in the upload you've > proposed. Somehow I didn't include this patch I submitted upstream in my last changes. This is done now[1]. > >> Anyway, I have just made a pull request to update this upstream[4] so >> hopefully the versioning will be sane. > > Thank you, and hopefully they'll agree. > >>>>>> * 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. > > Thanks. LTGM. P.S. If you wanted to "make a mark" on many packages, a > possible project could be to extend lintian to detect when > @ANY_VERSION@@ARCHIVE_EXT@ can be used, but isn't yet being used. I can probably open a wishlist bug for now and wait until I am more confident on my Perl to do the real work. > >>>>>> * Use xz compression in d/gbp.conf. > [snip] > >> I meant to say "gbp buildpackage" which uses this field to specify the >> compression type. Sorry for the confusion. > > No worries! > > [snip] > >> 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. > > I agree this makes sense, particularly when using a snapshot. > Personally I'm fine with the unreproducible orig.tar checksum when > packing a git treeish. I seem to remember that Sławomir Wójcik uploaded > the official upstream-released orig.tar.gz to his RFS, and I'd prefer > not to break his workflow, since he's the only human maintainer of this > package. If you'd like to add yourself to Uploaders then I'll accept > the switch to xz as well as other maintainer-level decisions you'd like > to make. After all, Wójcik hasn't worked on this package in almost > three years. Ack. Added myself to the uploader list as this seems to be permitted for team members[4]. > >> 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. > > Yes, that command will download the official release orig.tarball, > unpack it to the upstream branch, then merge the > upstream[via-tarball]/tag to the Debian branch; this is only used when > importing a new release. For a -2 revision, on uses uscan > --download-current-version. Ack. > >> 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, > > Close! To be fair, there are many workflows. If I remember correct the > current state of this repo is a manual tag-merging workflow without > dgit-maint-merge. Ack. > >> which I believe is incompatible with "gbp import-orig --uscan" approach >> which directly import the release tarball without the git history. > > Yes, that's correct for that command. Gbp can also import from tags, > and it has some kind of hybrid tarball + git history mode too, but I > can't remember ever using it. > >> 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. >> > > Git deborig uses git-archive, so the orig.tarball won't be reproducible. > > I've probably missed a few solutions, but the ones I know of are: > > * Gbp's optional pristine-tar support keeps deltas for every tarball > on a git branch, and pristine-tar is almost always reproducible--except > in extremely rare corner cases. > > * Origtargz uses a variety of methods (including checking for > pristine-tar and using it if available), but origtargz always puts the > tarball in ../ rather than in the configured orig.tarball dir; this is > fixable, of course, but no one has done it yet. > > * Then there's uscan. A common, and more old-school workflow is to > always call uscan. If the required orig.tar is absent, it will download > a copy. > > Gbp's "git-prebuild" config key with uscan should do the trick. And of > course there are always shell aliases as well as custom wrapper scripts. > I wrote a wrapper script that checks for all the things that I tend to > forget. It's nice to be able to type "build" or "release" and have it > do the right thing, even for an irregular case that one hasn't seen in a > year and a half. As it seems that the repo merges from upstream source directly, we seem to need a hybrid approach that manually check for upstream tarball availability if use a tagged release. I'll probably manually do the checking for now. All changes have been pushed to team repo and latest package uploaded to mentors. PTAL, TIA! > > Best, > Nicholas > [1] https://salsa.debian.org/emacsen-team/scala-mode-el/-/commit/a6f7a9d726b8c1b484bf4c28700643d9edbb156d [2] https://salsa.debian.org/emacsen-team/scala-mode-el/-/commit/f91169cf739e844dfb3bde19aa45e6db65c05a91 [3] https://salsa.debian.org/emacsen-team/scala-mode-el/-/commit/ab23cceeb7b82001784da823bc3d67bd076c6d2e [4] https://salsa.debian.org/emacsen-team/scala-mode-el/-/commit/b4effb76171f74336a31be1e156a4d0a95cafa9d -- Xiyue Deng
Attachment:
signature.asc
Description: PGP signature