Hi Hugo, it's always great to see you contribute :)
On Sun Aug 13, 2023 at 1:02 AM CEST, Hugo Osvaldo Barrera wrote:
> +.PHONY: build> +build: target/release/rss-email rss-email.1 rss-email.5
I'd rather have a separate `doc` rule that (optionally) builds the documentation
with scdoc, but seeing as it's standard to build documentation on invoking
`make` it's probably better to instead not mark scdoc as an optional dependency.
Can you remove the "optional" marker for scdoc in the README?
> +.PHONY: install> +install: build> + @install -Dm755 target/release/rss-email ${DESTDIR}${PREFIX}/bin/rss-email> + @install -Dm644 rss-email.1 ${DESTDIR}${PREFIX}/share/man/man1/rss-email.1> + @install -Dm644 rss-email.5 ${DESTDIR}${PREFIX}/share/man/man1/rss-email.5
`rss-email.5` needs to go to `${DESTDIR}${PREFIX}/share/man/man5`.
> + @install -Dm644 LICENSE ${DESTDIR}${PREFIX}/share/licenses/rss-email/LICENSE
`${PREFIX}/share/licenses` is the license directory on Alpine and Arch, for
example, but Debian expects the dep5 copyright file in
`${PREFIX}/share/doc/${PACKAGE}` without any original LICENSE file installed
anywhere.
Because of this the LICENSE ideally shouldn't be installed in the `install`
step and installation of it to the correct place (and in the correct format)
should be left to downstream IMO. Your opinion on this is appreciated, though!
Thank you for your contribution!
--
witcher
On Sun Aug 13, 2023 at 1:02 AM CEST, Hugo Osvaldo Barrera wrote:
> So it gets properly tested.> ---> Tested here: https://builds.sr.ht/~whynothugo/job/1040484> .build.yml | 8 +++++++-> 1 file changed, 7 insertions(+), 1 deletion(-)>> diff --git a/.build.yml b/.build.yml> index 400b19a..0b4856f 100644> --- a/.build.yml> +++ b/.build.yml> @@ -5,6 +5,7 @@ triggers:> to: witcher <witcher@wiredspace.de>> packages:> - rust> + - scdoc> sources:> - https://git.sr.ht/~witcher/rss-email> tasks:> @@ -19,4 +20,9 @@ tasks:> cargo clippy --frozen -- -D warnings> - build: |> cd rss-email> - cargo build --frozen> + # Building with --release is slow and pointless here.> + sed -i -e 's/--release //' -e 's/release/debug/' Makefile> + make build> + - installl: |
Typo: "installl" :)
> + cd rss-email> + sudo make install
Can you install this to a local directory with `DESTDIR` and use `tree` to
display the result in CI? Otherwise subtle mistakes can't be spotted in a CI
run.
Thank you again!
--
witcher
On Sun, 13 Aug 2023, at 09:50, witcher wrote:
> [...]> `${PREFIX}/share/licenses` is the license directory on Alpine and Arch, for> example, but Debian expects the dep5 copyright file in> `${PREFIX}/share/doc/${PACKAGE}` without any original LICENSE file installed> anywhere.> Because of this the LICENSE ideally shouldn't be installed in the `install`> step and installation of it to the correct place (and in the correct format)> should be left to downstream IMO. Your opinion on this is appreciated, though!>> Thank you for your contribution!>> -- > witcher>
Oh, I forgot to reply to this last item.
I'm not sure what's best. Either we can drop installing the licence (and
require that downstream always handles it themselves), or we can leave it
as-is (which helps some distros, but others have to move it elsewhere).
I'll leave that up to you :)
--
Hugo
Hi Hugo,
before applying your patches, I went ahead and corrected your commits to avoid
another round of patches for these trivial changes. I hope you don't mind.
Thank you for your contribution and maintaining the package for it for Alpine!
--
witcher
On 2023-08-21 20:56, witcher wrote:
> Hi Hugo,> > before applying your patches, I went ahead and corrected your commits to avoid> another round of patches for these trivial changes. I hope you don't mind.> > Thank you for your contribution and maintaining the package for it for Alpine!> > -- > witcher
No objections on my behalf, thanks for giving it the final polish.
Cheers,
--
Hugo