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

NobodyXu · 2024-09-21T07:06:58Z

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 [email protected]
Co-authored-by: Taiki Endo [email protected]

taiki-e · 2024-09-21T13:25:29Z

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 · 2024-09-21T13:39:43Z

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 · 2024-09-23T09:37:04Z

cc @taiki-e would that work for you?

NobodyXu · 2024-09-27T05:06:50Z

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

taiki-e · 2024-10-01T15:36:00Z

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 · 2024-10-01T15:50:24Z

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 · 2024-10-01T16:05:29Z

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 · 2024-10-05T02:25:31Z

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

NobodyXu · 2024-10-14T10:00:39Z

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

taiki-e · 2024-10-14T11:41:14Z

.github/workflows/manifest_sync.yml

+  workflow_call:
+  push:
+    branches:
+      - main


I would not prefer to do this on every push. An unreleased code may contain broken changes.

I changed it to run on release (pre-release/draft should be ignored)

taiki-e · 2024-10-14T11:45:14Z

tools/checkout-manifest-schema-branch.sh

+
+cd "$(dirname "$0")"
+
+schema_version="$(grep 'version = "0.*.0"' ./manifest-schema/Cargo.toml | cut -d '.' -f 2)"


This is very fragile. Any of the following cases break this script:

manifest-schema has non-zero patch version

dependency is <name> = { version = "..", .. } form

dependency is [dependencies.<name>] form

The most robust way would be to use cargo-metadata and cargo-locate-project (tools/tidy.sh has code that does such a thing), but a simple approach such as to use a marker to identify the lines we want to reference (like this) would be fine.

I replaced this with cargo-metadata and jq, I believe that's the most robust one without having to install third-party tool.

Thanks.

without having to install third-party tool.

(Just in case: cargo locate-project is not a third-party tool.)

Thanks, TIL

tools/codegen/Cargo.toml

tools/checkout-manifest-schema-branch.sh

taiki-e · 2024-10-14T13:34:42Z

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

NobodyXu · 2024-10-14T13:35:53Z

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 · 2024-10-14T23:36:56Z

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

NobodyXu · 2024-10-15T13:15:33Z

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 · 2024-10-21T10:15:50Z

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 · 2024-10-22T15:42:39Z

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

NobodyXu · 2024-12-04T22:57:18Z

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?

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 taiki-e-install-action-manifest-schema so that it could be publish it to crates-io

Signed-off-by: Jiahao XU <[email protected]>

Set username and user email before committing. Signed-off-by: Jiahao XU <[email protected]>

Signed-off-by: Jiahao XU <[email protected]>

Co-authored-by: Taiki Endo <[email protected]>

Signed-off-by: Jiahao XU <[email protected]>

Co-authored-by: Taiki Endo <[email protected]>

Signed-off-by: Jiahao XU <[email protected]>

NobodyXu · 2024-12-05T13:29:35Z

Rebased and resolved merge conflicts

taiki-e · 2024-12-08T13:29:26Z

how shall we proceed from here?

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

Signed-off-by: Jiahao XU <[email protected]>

NobodyXu · 2024-12-11T13:45:12Z

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

NobodyXu · 2024-12-19T02:01:11Z

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

The CI is green now

NobodyXu requested a review from taiki-e September 21, 2024 07:06

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

taiki-e reviewed Oct 14, 2024

View reviewed changes

tools/checkout-manifest-schema-branch.sh Outdated Show resolved Hide resolved

NobodyXu and others added 9 commits December 6, 2024 00:24

Set install-action-internal-codegen to publish=false

6bb5d04

Rename crate to install-action-manifest-schema

e0c5e01

Add GHA workflow manifest-sync.yml

f0e66bd

Signed-off-by: Jiahao XU <[email protected]>

Add MANIFEST_SCHEMA_BRANCH_NAME to manifest-schema

e24f809

Signed-off-by: Jiahao XU <[email protected]>

Fix Show diff step in manifest_sync.yml

b031775

Signed-off-by: Jiahao XU <[email protected]>

Fix checkout-manifest-schema-branch.sh

b4a1350

Set username and user email before committing. Signed-off-by: Jiahao XU <[email protected]>

Remove unused key in concurrency group

9b7e021

Signed-off-by: Jiahao XU <[email protected]>

Update tools/checkout-manifest-schema-branch.sh

ff2d7f4

Co-authored-by: Taiki Endo <[email protected]>

NobodyXu and others added 9 commits December 6, 2024 00:24

Run manifest_sync.yml on release

db1c9b7

Signed-off-by: Jiahao XU <[email protected]>

Revert unnecessary publish = false in codegen/Cargo.toml

e599d95

Signed-off-by: Jiahao XU <[email protected]>

Make getting version of install-action-manifest-schema more robust

8d29a4e

Signed-off-by: Jiahao XU <[email protected]>

Update tools/checkout-manifest-schema-branch.sh

7d69292

Co-authored-by: Taiki Endo <[email protected]>

Add license to install-action-manifest-schema

9d64072

Signed-off-by: Jiahao XU <[email protected]>

Fix checkout-manifest-schema-branch.sh fmt

0a08583

Signed-off-by: Jiahao XU <[email protected]>

Fix speeing

f9ae86c

Signed-off-by: Jiahao XU <[email protected]>

Add license line to checkout-manifest-schema-branch.sh

b2feb06

Signed-off-by: Jiahao XU <[email protected]>

Fix manifest_sync.yml: Limit permission contents: read

c270e26

Signed-off-by: Jiahao XU <[email protected]>

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

NobodyXu added 2 commits December 12, 2024 00:36

Fix file-permissions-check in tools/tidy.sh

d80da85

Signed-off-by: Jiahao XU <[email protected]>

Remove unused words from .github/.cspell/project-dictionary.txt

0b6e1a3

Signed-off-by: Jiahao XU <[email protected]>

NobodyXu requested a review from taiki-e December 11, 2024 13:41

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 enabled auto-merge (squash) December 11, 2024 13:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

NobodyXu commented Sep 21, 2024 •

edited

Loading

taiki-e commented Sep 21, 2024

NobodyXu commented Sep 21, 2024

NobodyXu commented Sep 23, 2024

NobodyXu commented Sep 27, 2024

taiki-e commented Oct 1, 2024

NobodyXu commented Oct 1, 2024

taiki-e commented Oct 1, 2024 •

edited

Loading

NobodyXu commented Oct 5, 2024

NobodyXu commented Oct 14, 2024

taiki-e Oct 14, 2024

NobodyXu Oct 14, 2024

taiki-e Oct 14, 2024

NobodyXu Oct 14, 2024

taiki-e Oct 14, 2024

NobodyXu Oct 14, 2024

taiki-e commented Oct 14, 2024

NobodyXu commented Oct 14, 2024

NobodyXu commented Oct 14, 2024

NobodyXu commented Oct 15, 2024 •

edited

Loading

NobodyXu commented Oct 21, 2024

taiki-e commented Oct 22, 2024 •

edited

Loading

NobodyXu commented Dec 4, 2024 •

edited

Loading

NobodyXu commented Dec 5, 2024

taiki-e commented Dec 8, 2024

NobodyXu commented Dec 11, 2024

NobodyXu commented Dec 19, 2024


		cd "$(dirname "$0")"

		schema_version="$(grep 'version = "0.*.0"' ./manifest-schema/Cargo.toml \| cut -d '.' -f 2)"

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

Are you sure you want to change the base?

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

Conversation

NobodyXu commented Sep 21, 2024 • edited Loading

taiki-e commented Sep 21, 2024

NobodyXu commented Sep 21, 2024

NobodyXu commented Sep 23, 2024

NobodyXu commented Sep 27, 2024

taiki-e commented Oct 1, 2024

NobodyXu commented Oct 1, 2024

taiki-e commented Oct 1, 2024 • edited Loading

NobodyXu commented Oct 5, 2024

NobodyXu commented Oct 14, 2024

taiki-e Oct 14, 2024

Choose a reason for hiding this comment

NobodyXu Oct 14, 2024

Choose a reason for hiding this comment

taiki-e Oct 14, 2024

Choose a reason for hiding this comment

NobodyXu Oct 14, 2024

Choose a reason for hiding this comment

taiki-e Oct 14, 2024

Choose a reason for hiding this comment

NobodyXu Oct 14, 2024

Choose a reason for hiding this comment

taiki-e commented Oct 14, 2024

NobodyXu commented Oct 14, 2024

NobodyXu commented Oct 14, 2024

NobodyXu commented Oct 15, 2024 • edited Loading

NobodyXu commented Oct 21, 2024

taiki-e commented Oct 22, 2024 • edited Loading

NobodyXu commented Dec 4, 2024 • edited Loading

NobodyXu commented Dec 5, 2024

taiki-e commented Dec 8, 2024

NobodyXu commented Dec 11, 2024

NobodyXu commented Dec 19, 2024

NobodyXu commented Sep 21, 2024 •

edited

Loading

taiki-e commented Oct 1, 2024 •

edited

Loading

NobodyXu commented Oct 15, 2024 •

edited

Loading

taiki-e commented Oct 22, 2024 •

edited

Loading

NobodyXu commented Dec 4, 2024 •

edited

Loading