dfinity / vessel

The original package manager for Motoko
Apache License 2.0
113 stars 19 forks source link

Fix 403 error when using moc compiler version > 0.6.2 #41

Closed ByronBecker closed 2 years ago

ByronBecker commented 2 years ago

Fix

TLDR: To fix 403 forbidden compiler release downloads for versions > 0.6.2 that hit GitHub releases, update the download_compiler reqwest.get to set a User-Agent header

Motivation

I was running into issues where locally, I'm able to build my code and use moc version 0.6.20, but my GitHub CI process runs into this forbidden error when trying to download the binary (note that I started with this template, and just update the compiler version https://github.com/kritzcreek/motoko-library-template).

I don't run into this same error when using moc 0.6.2 and below, and I had tried every other version above it, but I run into the same forbidden error with 0.6.11 and 0.6.19.

I dug into this further and saw that every version up to 0.6.2 was released on https://download.dfinity.systems/motoko/{version}/{os_arch}/motoko-{version}.tar.gz, but since then, from 0.6.3 up to 0.6.21 has been released on GitHub via the https://github.com/dfinity/motoko/releases/download/{version}/motoko-{os}-{version}.tar.gz.

Since I'm able to both locally (via vessel) and manually download (through the releases page) release binaries 0.6.3+ from the GitHub releases page, this means there's either a specific download permission setting setting that's blocking the GitHub CI role or the request vessel is making to download the specific moc compiler version was messing up.

The code where the vessel package manager is attempting to download the binary can be found here.

For reference, this is the error I was receiving in the CI logs:

Run make check-strict
Error: Failed to download Motoko binaries for version 0.6.20, with "403 Forbidden"

Details: <?xml version="1.0" encoding="UTF-8"?>
<Error><Code>AccessDenied</Code><Message>Access Denied</Message><RequestId>HWH6Y08H658Y6KBG</RequestId><HostId>NNRcEQKUFwjZOQNNQQnVhmj6prO/0/IZBou0bUiyeZOs7+sIMqbRlHNRCU/X7XlfNfJM1cFKkcA=</HostId></Error>
find src -type f -name '*.mo' -print0 | xargs -0 /moc --package base .vessel/base/e0c95f909e17c431921f1be35800242a6ddd4a55/src -Werror --check
xargs: /moc: No such file or directory
make: *** [Makefile:9: check-strict] Error 127

Where the check-strict command is

check-strict:
    find src -type f -name '*.mo' -print0 | xargs -0 $(shell vessel bin)/moc $(shell vessel sources) -Werror --check

And the ci yaml file looked like this https://github.com/kritzcreek/motoko-library-template/blob/main/.github/workflows/ci.yml

Since I was receiving the "Failed to download" error mentioned on [this line], I started there and found an issue where if the User-Agent header is not specified, the GitHub API will throw this error. You can see on this line that this header is set - and the API does not complain.

The Fix (and proof it works)

kritzcreek commented 2 years ago

Nice analysis, and the fix looks simple enough and straight forward. Looks good to me, thank you!

ByronBecker commented 2 years ago

Thanks for the review :), I believe I need a second one as a first time contributor to get this merged and then released @ninegua @nomeata

ByronBecker commented 2 years ago

@crusso @chenyan-dfinity

Sorry to bother, need one more approval on this quick bug fix (two approvals required)!

nomeata commented 2 years ago

You need approvals from actual current committees of this repo. It seems not nice that @kritzcreek got his permissions revoked, could someone at dfinity fix that? (Unless Christoph doesn't want them back, of course)

ByronBecker commented 2 years ago

@nomeata Who should you or I tag to reissue permission. I agree it would be nice to have the original owner of this package manager have the ability to merge in new features.

crusso commented 2 years ago

Thanks for your perseverance @ByronBecker! Did that do the trick?

ByronBecker commented 2 years ago

@crusso, it did - but looks like we're somehow failing CI now, which is odd to me considering where it's failing. If you don't mind - I'll take a look at it tomorrow and see if there's anything I can find (fix a hash or something like that)

ByronBecker commented 2 years ago

@crusso Actually, it looks like the current main fails these same checks. I'm wondering if we can just merge as is then, so I don't have to fix previous issues as well in this pull request except for the bug described above

ByronBecker commented 2 years ago

I don't have merge capabilities, so it would have to be someone currently working at Dfinity with those permissions

crusso commented 2 years ago

CI issue fixed here #42 (thanks to handholding by @kritzcreek)