Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH Action does not work with Alpine #769

Open
mvorisek opened this issue Jun 26, 2023 · 18 comments
Open

GH Action does not work with Alpine #769

mvorisek opened this issue Jun 26, 2023 · 18 comments
Labels
question Uncertainty is involved

Comments

@mvorisek
Copy link

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
Copy link
Collaborator

epage commented Jun 26, 2023

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
Copy link
Author

mvorisek commented Jun 26, 2023

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
Copy link
Collaborator

epage commented Jun 26, 2023

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
Copy link
Author

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
Copy link
Collaborator

epage commented Jun 26, 2023

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
Copy link
Author

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
Copy link
Author

mvorisek commented Jun 27, 2023

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
Copy link
Collaborator

epage commented Jun 27, 2023

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
Copy link
Author

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
Copy link
Collaborator

epage commented Jun 27, 2023

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
Copy link
Author

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
Copy link
Collaborator

epage commented Jun 27, 2023

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
Copy link
Author

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
Copy link
Contributor

not-my-profile commented Jun 27, 2023

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
Copy link
Author

mvorisek commented Jul 5, 2023

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
Copy link
Collaborator

epage commented Jul 5, 2023

  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
Copy link
Author

mvorisek commented Jul 5, 2023

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 hendrikmuhs/ccache-action#145?

@epage
Copy link
Collaborator

epage commented Jul 14, 2023

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.

@epage epage added the question Uncertainty is involved label Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Uncertainty is involved
Projects
None yet
3 participants