search

shadow-maint / shadow

Upstream shadow tree
Other
287 stars 227 forks source link

Prepare tests/ for debian CI #1003

Closed hallyn closed 2 weeks ago

hallyn commented 1 month ago

A few changes which I had to make in

https://salsa.debian.org/debian/shadow/-/merge_requests/21

I'd like to make them here instead, then, on each release, export a tests.tar.gz tarball which debian can import.

zeha commented 2 weeks ago

Based on @hallyn's work earlier I've opened https://salsa.debian.org/debian/shadow/-/merge_requests/22/diffs for Debian. Unfortunately the work is stalled while linux upstream figures out how to fix 9pfs (used in our test runner).

alejandro-colomar commented 2 weeks ago

On Mon, Jun 24, 2024 at 10:11:33AM GMT, Chris Hofstaedtler wrote:

Based on @hallyn's work earlier I've opened https://salsa.debian.org/debian/shadow/-/merge_requests/22/diffs for Debian. Unfortunately the work is stalled while linux upstream figures out how to fix 9pfs (used in our test runner).

Hi Serge,

-- Reply to this email directly or view it on GitHub: https://github.com/shadow-maint/shadow/pull/1003#issuecomment-2187037412 You are receiving this because your review was requested.

Do I need to check anything on that PR, or are you still working on it?

Have a lovely night! Alex

Message ID: @.***>

-- https://www.alejandro-colomar.es/

alejandro-colomar commented 2 weeks ago

Hi Serge,

On Mon, Jun 24, 2024 at 10:10:41AM GMT, Chris Hofstaedtler wrote:

@zeha commented on this pull request.

@@ -1,7 +1,5 @@

!/bin/sh

-set -x

I'm quite sure removing -x will not be necessary.

This is something I'd like you to re-check, since it seems weird that you need to remove set -x.

However, if you confirm that you don't seem to make it work otherwise, I'm ready to merge this, and let us find a better solution/explanation in the future.

-- Reply to this email directly or view it on GitHub: https://github.com/shadow-maint/shadow/pull/1003#discussion_r1651363178 You are receiving this because your review was requested.

Message ID: @.***>

Have a lovely day! Alex

-- https://www.alejandro-colomar.es/

hallyn commented 2 weeks ago

Hi Serge, On Mon, Jun 24, 2024 at 10:10:41AM GMT, Chris Hofstaedtler wrote: @zeha commented on this pull request. > @@ -1,7 +1,5 @@ #!/bin/sh -set -x I'm quite sure removing -x will not be necessary. This is something I'd like you to re-check, since it seems weird that you need to remove set -x. However, if you confirm that you don't seem to make it work otherwise, I'm ready to merge this, and let us find a better solution/explanation in the future.

Well, FWIW adding 'set -x' is usually seen as a bad thing, to be used for debugging only. I'm used to having to defend adding, not removing, 'set -x' :)

alejandro-colomar commented 2 weeks ago

On Wed, Jun 26, 2024 at 08:49:13AM GMT, Serge Hallyn wrote:

Hi Serge, On Mon, Jun 24, 2024 at 10:10:41AM GMT, Chris Hofstaedtler wrote: @zeha commented on this pull request. > @@ -1,7 +1,5 @@ #!/bin/sh -set -x I'm quite sure removing -x will not be necessary. This is something I'd like you to re-check, since it seems weird that you need to remove set -x. However, if you confirm that you don't seem to make it work otherwise, I'm ready to merge this, and let us find a better solution/explanation in the future.

Well, FWIW adding 'set -x' is usually seen as a bad thing, to be used for debugging only. I'm used to having to defend adding, not removing, 'set -x' :)

I agree. I'd like to remove it, but I'd like to do it for the right reasons. That is, something in my head is saying that we might be silencing some deeper problem that might bite us in the future if we don't really understand it.

-- Reply to this email directly or view it on GitHub: https://github.com/shadow-maint/shadow/pull/1003#issuecomment-2192041690 You are receiving this because your review was requested.

Message ID: @.***>

-- https://www.alejandro-colomar.es/

alejandro-colomar commented 2 weeks ago

I've dropped the set -x patch.

$ git range-diff 7d304a14^ 6fbf4ed5 7d304a14
1:  7d304a14 = 1:  7d304a14 tests: support run_some from exported tarball
2:  6fbf4ed5 < -:  -------- tests: remove -x from some tests
alejandro-colomar commented 2 weeks ago

Rebased and signed

$ git range-diff 7d304a14^..7d304a14 shadow/master..HEAD
1:  7d304a14 ! 1:  8763bc26 tests: support run_some from exported tarball
    @@ Metadata
     Author: Serge Hallyn <serge@hallyn.com>

      ## Commit message ##
    -    tests: support run_some from exported tarball
    +    tests/: Support run_some from exported tarball

         common/config.sh currently tries to find the top directory by looking
         for .git.  There are also many places under tests/ where we use
    @@ Commit message
         Changelog:
           2024 05 26: Incorporate feedback from alejandro-colomar

    +    Link: <https://salsa.debian.org/debian/shadow/-/merge_requests/21>
    +    Link: <https://salsa.debian.org/debian/shadow/-/merge_requests/22>
    +    Cc: Chris Hofstaedtler <zeha@debian.org>
         Signed-off-by: Serge Hallyn <serge@hallyn.com>
    +    Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## tests/common/config.sh ##
     @@
alejandro-colomar commented 2 weeks ago

The issues with CI are known and accepted. I'll merge, and we'll fix that later (the set -x issue), as agreed with @hallyn via email.