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

ci: release assets cannot be downloaded by install-filcrypto #491

Closed
rvagg opened this issue Sep 18, 2024 · 8 comments · Fixed by #492
Closed

ci: release assets cannot be downloaded by install-filcrypto #491

rvagg opened this issue Sep 18, 2024 · 8 comments · Fixed by #492

Comments

@rvagg
Copy link
Member

rvagg commented Sep 18, 2024

Follow-up to #486 but slightly different.

We now have compiled bundles uploaded to the release, as per the fix in #486, e.g. https://github.com/filecoin-project/filecoin-ffi/releases/tag/v1.30.0-dev2

But previously, they were uploaded to a release that was named after the first 16 characters of the commit hash. e.g.

Screenshot 2024-09-18 at 11 53 36 AM

This is then used by the install script to download the assets:

local __release_sha1=$(git rev-parse HEAD)
local __release_tag="${__release_sha1:0:16}"
local __release_tag_url="https://api.github.com/repos/filecoin-project/${__repo_name}/releases/tags/${__release_tag}"

I'm not actually sure of the reason behind this and I'm a little hesitant to change that behaviour (Chesterton's Fence), but I'll open a PR to at least explore changing the install script. But if we want to follow a least-change approach then we should probably be making a release with the 16 chars of the commit hash.

rvagg added a commit that referenced this issue Sep 18, 2024
Avoid the need for using the commit hash to find the release.

Fixes: #491
@rvagg
Copy link
Member Author

rvagg commented Sep 18, 2024

I think the reason for using the commit is that they were created for every commit made to master, thereby avoiding the need for everyone to compile from source if they used an untagged version:

Screenshot 2024-09-18 at 12 08 15 PM

So we need to decide if we're OK not doing that anymore.

@rvagg
Copy link
Member Author

rvagg commented Sep 18, 2024

#492 is a potential fix if we're going to lock in tag-only assets

rvagg added a commit that referenced this issue Sep 18, 2024
Avoid the need for using the commit hash to find the release.

Fixes: #491
@rjan90
Copy link
Contributor

rjan90 commented Sep 18, 2024

I do not have any strong opinions if it is worth changing this behaviour, and as such it might be better just to keep to what we have today.

@magik6k / @LexLuthr as consumers of FFI, do you have any preferences on this? I.e assets are uploaded to the tag, or the assets are uploaded to a release that was named after the first 16 characters of the commit hash?

@rvagg
Copy link
Member Author

rvagg commented Sep 18, 2024

and as such it might be better just to keep to what we have today

We don't have it today unfortunately because we removed the per-commit release creation when we removed CircleCI. So if we want to do it that way then we have to do some more GHA magic to make a release per commit.

@Stebalien
Copy link
Member

IIRC, we kept it to reduce the breakage. But I don't believe there was a strong reason to keep it that way (@cryptonemo may know more).

@galargh
Copy link
Contributor

galargh commented Sep 20, 2024

and as such it might be better just to keep to what we have today

We don't have it today unfortunately because we removed the per-commit release creation when we removed CircleCI. So if we want to do it that way then we have to do some more GHA magic to make a release per commit.

If we wanted to set up releases straight from master, I think we should be able to get that done in about 1 day if you wanted us to take care of it.

We could also try to revert https://github.com/filecoin-project/filecoin-ffi/pull/475/files but that would still need a couple of fixes to the publish script as it has some references to circleci that we later updated.

@rjan90
Copy link
Contributor

rjan90 commented Sep 20, 2024

After verbal discussions with @BigLep, we've reached a conclusion regarding the FFI asset upload process:

We believe it's appropriate to proceed with locking in tag-only assets, given the lack of pushback on this approach. This decision is primarily driven by:

  1. The current absence of FFI assets causing significant inconvenience.
  2. The need for a timely resolution to this issue.

While we acknowledge this change may impact workflows relying on per-commit releases, such concerns haven't been voiced, and per-commit releases haven't occurred since 2024-07. The benefits of quickly concluding this matter appear to outweigh potential drawbacks.

To address FFI asset generation time concerns, we've discussed exploring more powerful custom runners for asset creation when needed.

Unless there are strong objections, I propose we move forward with the changes outlined in #492 on 2024-09-24, implementing the tag-only asset approach.

@BigLep BigLep added this to FilOz Sep 24, 2024
@github-project-automation github-project-automation bot moved this to 📌 Triage in FilOz Sep 24, 2024
@rjan90 rjan90 moved this from 📌 Triage to ✔️ Approved by reviewer in FilOz Sep 24, 2024
@rjan90 rjan90 moved this from ✔️ Approved by reviewer to ⌨️ In Progress in FilOz Sep 24, 2024
rvagg added a commit that referenced this issue Sep 24, 2024
Avoid the need for using the commit hash to find the release.

Fixes: #491
@github-project-automation github-project-automation bot moved this from ⌨️ In Progress to 🎉 Done in FilOz Sep 24, 2024
@rvagg
Copy link
Member Author

rvagg commented Sep 24, 2024

done, but we'll need another tag now @rjan90

@rjan90 rjan90 moved this from 🎉 Done to ☑️ Done (Archive) in FilOz Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ☑️ Done (Archive)
Development

Successfully merging a pull request may close this issue.

4 participants