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

Extract install-action-manifest-schema and publish to crates-io #657

Merged
merged 39 commits into from
Jan 28, 2025

Conversation

NobodyXu
Copy link
Collaborator

@NobodyXu NobodyXu commented Sep 21, 2024

Motivation:

In cargo-bins/cargo-binstall#1720, I'd like to speedup cargo-binstall, by using the template recorded in taiki-e/install-action while utilise the checksum to validate the artifacts.

I've extracted all manifests I need into a crate named install-action-manifest-schema so that it could be publish it to crates-io

Also:

  • Fix file-permissions-check in tools/tidy.sh
  • Remove unused words from .github/.cspell/project-dictionary.txt

Signed-off-by: Jiahao XU Jiahao_XU@outlook.com
Co-authored-by: Taiki Endo te316e89@gmail.com

@NobodyXu NobodyXu requested a review from taiki-e September 21, 2024 07:06
@taiki-e
Copy link
Owner

taiki-e commented Sep 21, 2024

Could you tell us how specifically you plan to embed this into binstall?

For example, if binstall refers to it by tag or rev, no problem, but otherwise maybe versioning or stabilizing our manifest format foever is needed.

@NobodyXu
Copy link
Collaborator Author

Could you tell us how specifically you plan to embed this into binstall?

I plan to issue a GET request, to get the raw JSON from the repository, since I don't want to create new release of cargo-bindtall too frequently

For example, if binstall refers to it by tag or rev, no problem, but otherwise maybe versioning or stabilizing our manifest format foever is needed.

Yeah having a branch for cargo-binstall would be great, but it'd be also fine without it.

I don't think the format will change very often, and I plan to use the manifest as a best-effort, though having it in a separate branch would be great.

@NobodyXu
Copy link
Collaborator Author

cc @taiki-e would that work for you?

@NobodyXu
Copy link
Collaborator Author

@taiki-e Friendly ping on this PR for it has been stale for days.

@taiki-e
Copy link
Owner

taiki-e commented Oct 1, 2024

To be honest, I'm not much in favor of merging as is, as I expect that people will eventually require that checksum verification be done for supported tools reliably.

What I think is right here is to provide a versioned manifest from the beginning.

The idea I'm currently considering is as follows:

  • Create branches for manifest-schema.
    • The format of the branch would be something like manifest-schema-<major_and_minor_version_of_install_action_manifest_schema_crate>.
  • When a new version of install-action is released, the release script will also update the corresponding manifest-schema- branch based on the current install-action-manifest-schema crate version.
    • This means that the manifest-schema- branch will contain the latest released manifest that uses a compatible format.
    • Also, if there are breaking changes to manifest format, we must publish a new version of the crate first.
  • install-action-manifest-schema crate provides a constant indicating the name of the corresponding manifest-schema- branch based on the current version of that crate.
    • Users (binstall) can use it like format!("https://.../{}/...", install_action_manifest_schema::BRANCH_NAME)

Thoughts?


P.S.

Friendly ping on this PR for it has been stale for days.

In general, I don't recommend pinging at intervals shorter than a week in OSS. I understand your feeling that you want the PRs to merge as soon as possible, but the maintainers have lives of their own and are not always able to be involved in OSS as often.

"a week" is what I have said in another place before, and also a guide to new contributors in LLVM recommends the same interval.

The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

@NobodyXu
Copy link
Collaborator Author

NobodyXu commented Oct 1, 2024

In general that looks pretty good to me.

I think that we only need the major version in the branch name, i.e. manifest-schema-1

The reason is that, older version of cargo-binstall might still uses older version of manifest-schema.

As long as it's compatible (adding new field), I think it's probably ok.


I'm sorry for the pinging, I would refrain from pinging afterwards unless it's longer than 8 days.

@taiki-e
Copy link
Owner

taiki-e commented Oct 1, 2024

I think that we only need the major version in the branch name, i.e. manifest-schema-1

I said major_and_minor_version because I was speaking under the assumption that the major version of the install-action-manifest-schema crate would be 0 forever, but if the major version is 1 or higher, indeed the major version alone would be sufficient.

As long as it's compatible (adding new field), I think it's probably ok.

Agreed.

@NobodyXu NobodyXu force-pushed the refactor/new-crate-for-parsing-manifest branch 2 times, most recently from f9fda5b to 97512de Compare October 2, 2024 14:58
@NobodyXu
Copy link
Collaborator Author

NobodyXu commented Oct 5, 2024

Added a new GHA workflow for the synchronisation, and added a new constant to the manifest-schema crate for the branch name.

@NobodyXu
Copy link
Collaborator Author

cc @taiki-e pinging as this PR has been stale for 9 days

I've added the workflow required to sync the manifest to the manifest branch, triggered when pushed to main

.github/workflows/manifest_sync.yml Outdated Show resolved Hide resolved
tools/checkout-manifest-schema-branch.sh Outdated Show resolved Hide resolved
tools/codegen/Cargo.toml Outdated Show resolved Hide resolved
tools/checkout-manifest-schema-branch.sh Outdated Show resolved Hide resolved
@taiki-e
Copy link
Owner

taiki-e commented Oct 14, 2024

tidy failures seem to be genuine except for file-permissions-check.

@NobodyXu
Copy link
Collaborator Author

tidy failures seem to be genuine except for file-permissions-check.

Hmmm how do I fix it?

I took a look at its err msg and didn't get how to fix it

@NobodyXu
Copy link
Collaborator Author

Oh i get it now, last time I looked at it I was confused

@NobodyXu
Copy link
Collaborator Author

NobodyXu commented Oct 15, 2024

I've managed to fix all other errors, but I don't know how to fix this one:

https://github.com/taiki-e/install-action/actions/runs/11346131368/job/31554551380?pr=657#step:10:42

info: checking public crates don't contain executables and binaries
Error: file-permissions-check failed: executables are only allowed to be present in directories that are excluded from crates.io
=======================================
main.sh
=======================================

@NobodyXu
Copy link
Collaborator Author

cc @taiki-e needs some help with this error, since I didn't touch main.sh

Tried putting exclude into the new crate I created but doesn't help.

@taiki-e
Copy link
Owner

taiki-e commented Oct 22, 2024

As I said above (#657 (comment)), that is spurious. It is still a marge blocker, but it is not your fault.

@NobodyXu
Copy link
Collaborator Author

NobodyXu commented Dec 4, 2024

cc @taiki-e how shall we proceed from here?

Maybe you can force merge it, if only the merge commit for this PR will fail and others will continue to work?

@NobodyXu NobodyXu force-pushed the refactor/new-crate-for-parsing-manifest branch from d38bc38 to c270e26 Compare December 5, 2024 13:24
@NobodyXu
Copy link
Collaborator Author

NobodyXu commented Dec 5, 2024

Rebased and resolved merge conflicts

@taiki-e
Copy link
Owner

taiki-e commented Dec 8, 2024

how shall we proceed from here?

I don't think there is any other way than to adjust the tidy script (tools/tidy.sh).

@NobodyXu NobodyXu requested a review from taiki-e December 11, 2024 13:41
@NobodyXu NobodyXu changed the title Extract manifest-schema and publish to crates-io Extract install-action-manifest-schema and publish to crates-io Dec 11, 2024
@NobodyXu NobodyXu enabled auto-merge (squash) December 11, 2024 13:44
@NobodyXu
Copy link
Collaborator Author

I have fixed the tools/tidy.sh as suggested, please review again when you have time.

@NobodyXu
Copy link
Collaborator Author

cc @taiki-e can you please review this PR?

The CI is green now

@NobodyXu NobodyXu disabled auto-merge January 6, 2025 13:20

Verified

This commit was signed with the committer’s verified signature.
kgarner7 Kendall Garner
tools/manifest-schema/src/lib.rs Outdated Show resolved Hide resolved
tools/ci/checkout-manifest-schema-branch.sh Outdated Show resolved Hide resolved
@NobodyXu NobodyXu disabled auto-merge January 9, 2025 18:59
Use branch name `manifest-schema-<major>.<minor>` until manifest-schema crate become 1.x
Copy link
Owner

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for working on this @NobodyXu!

@taiki-e taiki-e enabled auto-merge (squash) January 28, 2025 07:02
@taiki-e taiki-e merged commit 1ef1e14 into main Jan 28, 2025
50 checks passed
@taiki-e taiki-e deleted the refactor/new-crate-for-parsing-manifest branch January 28, 2025 07:02
@taiki-e
Copy link
Owner

taiki-e commented Jan 28, 2025

@taiki-e
Copy link
Owner

taiki-e commented Jan 28, 2025

The workflow was not triggered after all (probably because an event triggered using GITHUB_TOKEN (“release” in this case) cannot trigger an action), so I removed it and changed it to run as part of the tools/publish.sh instead (544f616). It is used in the 2.47.30 release and works fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants