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

Add CI Job to check generate file changes #438

Merged
merged 7 commits into from
Feb 21, 2024

Conversation

cylim
Copy link
Contributor

@cylim cylim commented Feb 18, 2024

Having issues to generate internet_identity.did locally, the checks.yml scripts should be fine. PR to test.

probably related:

export const initIdentity = (mainnet) => {
    const file = `/Users/daviddalbusco/.config/dfx/identity/${
        mainnet ? 'juno' : 'default'
    }/identity.pem`;

https://github.com/junobuild/juno/blob/f8e20817885f5954c681d18c846777e0bae953b5/scripts/identity.utils.mjs#L32:L35

ref: #338

@peterpeterparker
Copy link
Contributor

Thanks, super cool!

Now that we run the checks I realize both candid-extractor and didc needs to be available in the action to run the checks. I realize it's a bit more complicated than expected.

@cylim
Copy link
Contributor Author

cylim commented Feb 18, 2024

@peterpeterparker
candid-extractor can added into the Cargo.toml, since it is downaloadable with cargo.
Let's me see how to use didc-linux in Github Action.

another thing is the initIdentity(network) is using file from local machine (specifically macos), even if I fixed the above two issues, I am not sure how to handle this one.

@peterpeterparker
Copy link
Contributor

Let's me see how to use didc-linux in Github Action.

🤞

another thing is the initIdentity(network) is using file from local machine

We can skip generating internet_identity.did given that it's a third party dependency. Not sure why initIdentity is used for the generate script, that should not be the case 🤔.

In any case, I except the new action to detect differences even once we have added didc and candid_extractor. I'll fix those once we reach that point, I can have a look to the initIdentity at the same time.

@peterpeterparker peterpeterparker linked an issue Feb 18, 2024 that may be closed by this pull request
@cylim
Copy link
Contributor Author

cylim commented Feb 18, 2024

In did.idl.sh, added a variable to check if it is from GITHUB CI, then ignore index.did, ledger.did and internet_identity.did, due to those files are git ignored.

Reasons might failed, I am not sure whether I should commit those.

  1. in my local machine, it added below line to every *.did.d.ts files,
export declare const init: ({ IDL }: { IDL: IDL }) => IDL.Type[];
  1. satelite.did build_version code moved to satellite_extension.did.

@peterpeterparker
Copy link
Contributor

added a variable to check if it is from GITHUB CI, then ignore

Good idea. 👍

satelite.did build_version code moved to

That's expected, it's a good test for this PR. 😉
I have to upgrade few things on my machine and I'll fix that later today.

@peterpeterparker
Copy link
Contributor

@cylim I think I fixed the issue:

  1. didc used to generate the did files in the repo was version v0.3.5 while the CI job used last version, v0.3.6, therefore there was the init: (... differences. I updated the files.

  2. satellite.did is something related to Juno, I also updated this.

Following those changes I updated the branch and the CI is green 👍

Before merging the PR, I wanted to ask if you we shouldn't cache the Rust compiled files, WDYT?

For example with https://github.com/actions/cache/blob/main/examples.md#rust---cargo

@cylim
Copy link
Contributor Author

cylim commented Feb 20, 2024

@peterpeterparker pushed new version of checks.yml. Unsure how much time it might save in CI Job.

@peterpeterparker
Copy link
Contributor

I re-run a job and the cache worked well, spare half of the time or something. Let's go, we can always iterate if necessary.

Thanks a lot for the improvements and PR!

@peterpeterparker peterpeterparker merged commit 6dc6b2e into junobuild:main Feb 21, 2024
4 checks passed
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.

CI job that run npm run generate and checks for difference
2 participants