crate-ci / typos

Source code spell checker
Apache License 2.0
2.36k stars 92 forks source link

GH Action does not work with Alpine #769

Open mvorisek opened 1 year ago

mvorisek commented 1 year ago

repro:

name: Unit

on:
  push:

jobs:
  smoke-test:
    name: Smoke
    runs-on: ubuntu-latest
    container:
      image: ghcr.io/mvorisek/image-php:${{ matrix.php }}
    strategy:
      fail-fast: false
      matrix:
        php: ['latest']
        type: ['Phpunit']
        include:
          - php: 'latest'
            type: 'CodingStyle'
    steps:
      - name: Checkout
        uses: actions/checkout@v3

      - name: Check Spelling (only for CodingStyle)
        if: matrix.type == 'CodingStyle'
        uses: crate-ci/typos@master

CI result: https://github.com/atk4/ui/actions/runs/5378282318/jobs/9757872912#step:10:19

 Downloading 'typos' v1.15.5
wget: unrecognized option: progress=dot:mega
BusyBox v1.36.1 (2023-06-02 00:42:02 UTC) multi-call binary.

Usage: wget [-cqS] [--spider] [-O FILE] [-o LOGFILE] [--header STR]
    [--post-data STR | --post-file FILE] [-Y on/off]
    [-P DIR] [-U AGENT] [-T SEC] URL...

Retrieve files via HTTP or FTP

    --spider    Only check URL existence: $? is 0 if exists
    --header STR    Add STR (of form 'header: value') to headers
    --post-data STR Send STR using POST method
    --post-file FILE    Send FILE using POST method
    -c      Continue retrieval of aborted transfer
    -q      Quiet
    -P DIR      Save to DIR (default .)
    -S          Show server response
    -T SEC      Network read timeout is SEC seconds
    -O FILE     Save to FILE ('-' for stdout)
    -o LOGFILE  Log messages to FILE
    -U STR      Use STR for User-Agent header
    -Y on/off   Use proxy

it seems you use non-POSIX wget progress option.

epage commented 1 year ago

it seems you use non-POSIX wget progress option.

As far as I can tell, wget is not POSIX in the first place but purely GNU. I'm not too sympathetic if busybox re-implements a subset of the canonical version of a command. In this case, I believe you can install the regular version of wget to make it work

mvorisek commented 1 year ago

I cannot install regular wget easily. I expect this package to work with busubox/Alpine wget out of the box. The progress option should be probably dropped.

epage commented 1 year ago

It was added for #714.

I cannot install regular wget easily. I

Could you give more details? I thought its available within Alpine Linux?

What I'm needing to understand with this is why a non-conformant polyfills for the canonical implementation of a tool should be supported.

mvorisek commented 1 year ago

AFAK the package /wo preogress option is regular Alpine package - https://pkgs.alpinelinux.org/package/v3.18/main/x86_64/wget - compiling or downloading a non-standard/"full" wget would be very complicated.

I think some Alpine cond can be added like: if [ -f /etc/alpine-release ]; then XXX; else YYY; fi.

Where is action/entrypoint.sh tested, can a test with Alpine be added easily?

epage commented 1 year ago

Looking at the APKBUILD file for the wget package you linked is getting the source from a GNU FTP server and building it which should then have the progress bars as far as I can tell. I did not see any conditional compilation around progress bars in the GNU wget source code. What doesn't have dot progress bar support is GNU wget2.

mvorisek commented 1 year ago

Well, the version is in the releases https://github.com/crate-ci/typos/blob/v1.15.7/action/entrypoint.sh#L37 hardcoded anyway, what about commiting the build in the release itself /wo any download? Then the GH Action downloaded will handle the download in the most robust way by itself.

mvorisek commented 1 year ago

Here is what non-native/Alpine default wget prints for help (--version is not supported at all):

/ # wget --help
BusyBox v1.36.1 (2023-06-02 00:42:02 UTC) multi-call binary.

Usage: wget [-cqS] [--spider] [-O FILE] [-o LOGFILE] [--header STR]
        [--post-data STR | --post-file FILE] [-Y on/off]
        [-P DIR] [-U AGENT] [-T SEC] URL...

Retrieve files via HTTP or FTP

        --spider        Only check URL existence: $? is 0 if exists
        --header STR    Add STR (of form 'header: value') to headers
        --post-data STR Send STR using POST method
        --post-file FILE        Send FILE using POST method
        -c              Continue retrieval of aborted transfer
        -q              Quiet
        -P DIR          Save to DIR (default .)
        -S              Show server response
        -T SEC          Network read timeout is SEC seconds
        -O FILE         Save to FILE ('-' for stdout)
        -o LOGFILE      Log messages to FILE
        -U STR          Use STR for User-Agent header
        -Y on/off       Use proxy

Are you ok to match the support based on the 1st line/version?

epage commented 1 year ago

Well, the version is in the releases https://github.com/crate-ci/typos/blob/v1.15.7/action/entrypoint.sh#L37 hardcoded anyway, what about commiting the build in the release itself /wo any download? Then the GH Action downloaded will handle the download in the most robust way by itself.

Could you clarify what you mean?

Are you ok to match the support based on the 1st line/version?

Not quite a fan of programmatically processing human output

mvorisek commented 1 year ago

Not quite a fan of programmatically processing human output

Me neither, but it should be not a problem, as long as the "standard" alternative represents a fallback.

In order to use this tool without too much CI code, I need to be able to use it out of the box on Alpine. So what is the best way to fix it /wo requiring to install GNU wget?

epage commented 1 year ago

Could you clarify why installing GNU wget is insufficient? That seems like the natural workaround but no reason has been given for why it doesn't apply in your situation.

mvorisek commented 1 year ago

Because we want to have out CI setup clean as possible. When we will install wget, we will have to have multiple CI steps.

The same question, why this tool should not work with Alpine wget?

epage commented 1 year ago

The same question, why this tool should not work with Alpine wget?

  1. The more things we support, the more burden it puts on a project
  2. If there isn't a reasonable way to detect it
  3. As I've already said, its a non-conformant variant of an existing command. The burden is on Busybox and people using it, not on the projects trying to use wget
mvorisek commented 1 year ago

We however have full curl installed. What about checking if curl is installed first and using it instead of wget?

The check of curl binary will be clean and in theory, this will help also non-Alpine users.

not-my-profile commented 1 year ago

Alpine Linux is very common in CIs so I think a check as I introduced in #772 would spare typos users from this pitfall. I think checking "$(readlink "$(which wget)")" != /bin/busybox is quite a reasonable way of detecting busybox. I think that busybox doesn't support many of the GNU options is by design ... this is not something that will be fixed by busybox. And since it's the default in Alpine Linux people are likely to use busybox without even realizing it (until something breaks). I think what matters more than ideological arguments in this case is that the typos action works out of the box in common CI environments for a better user experience.

mvorisek commented 1 year ago

We however have full curl installed. What about checking if curl is installed first and using it instead of wget?

The check of curl binary will be clean and in theory, this will help also non-Alpine users.

@epage wdyt about this solution, ie. try to use curl when available first?

epage commented 1 year ago
  1. It would add complexity which increases risk (just like supporting both styles of wget)
  2. We would need to test in both cases (and make sure we actually are covering both cases)

We could possibly switch completely curl, depending on how the output is seeing as we made the change to intentionally improve output for some of our users.

However, I've still not seen an explanation justifying a change. Why can't the actual wget be installed instead of the knock off version or alternative tools?

mvorisek commented 1 year ago

Any extra install requirement is degrading the UX, it costs an extra bandwidth, config LoC and possibly requires cleanup if they are additional CI steps.

From your words, it seems you are worried about mainly about additional LoC in this project. Would you accept a PR if the PR will contain CI testing for both like in my ccache https://github.com/hendrikmuhs/ccache-action/pull/145?

epage commented 1 year ago

Any extra install requirement is degrading the UX, it costs an extra bandwidth, config LoC and possibly requires cleanup if they are additional CI steps.

Can you quantify this? As an outsider to this problem, this feels like over-optimizing something that doesn't matter

it seems you are worried about mainly about additional LoC in this project.

That isn't what I said. Frankly, this makes me tempted to close this issue due to a lack of being productive because I feel like participants have not been engaging with good faith but just repeatedly push what they want without communicating the motivations.