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

Submodule conversion #89

Merged
merged 8 commits into from
Sep 12, 2024
Merged

Submodule conversion #89

merged 8 commits into from
Sep 12, 2024

Conversation

grod220
Copy link
Contributor

@grod220 grod220 commented Sep 12, 2024

Closes #5
Closes #85

The initial motivation to use http requests has proven to be more finicky than hoped for. It introduces failure possibility during runtime when the submodule strategy pulls those issues in only after the cosmos registry dep is updated. This will allow us to have deterministic builds for the current commit and worry less about the cargo run command failing when we are trying to add another chain id. The downside is that it requires 150mb download when pulling down for the first time.

Copy link

changeset-bot bot commented Sep 12, 2024

⚠️ No Changeset found

Latest commit: eadfd75

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Comment on lines 62 to 81
execute_and_check:
name: Generate registry and Check for Diffs
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
with:
submodules: 'true'
- uses: actions-rs/toolchain@v1
with:
profile: minimal
toolchain: stable
override: true
- name: Generate registry
run: cargo run
env:
RUST_LOG: info
working-directory: tools/compiler
- name: Check for Uncommitted Changes
run: git diff --exit-code
working-directory: tools/compiler
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Allows us now to ensure folks are properly running the script and not doing manual edits

npm/package.json Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cutting a new release for consumers using bundled registries

@@ -1,4 +1,4 @@
import * as Deimos8 from '../../registry/chains/penumbra-testnet-deimos-8.json';
import * as Deimos8 from '../../registry/chains/penumbra-testnet-deimos-8-x6de97e39.json';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like the penumbra-testnet-deimos-8.json has been removed from the inputs. To pass the test, these need to be updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Related to #73, last time L and I were debugging. IIRC we were surprised to see that the relevant codepaths were dropping the -x6de97e39 and trying to import the suffix-less JSON file, which is not what I wanted. I suspect this is still an issue, but we can follow up and file if testing surfaces it again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed offline: made an update to the chain id swapping logic to only do so if the exact match fails

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Diff due to pulling from the latest commit from the cosmos registry

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think anyone is depending on this (?). But the input that generates this seems to have been deleted a while ago.

Comment on lines -11 to -18
futures = "0.3.30"
serde = { version = "1.0.204", features = ["derive"] }
serde_json = "1.0.120"
regress = "0.10.0"
reqwest = { version = "0.12.5", features = ["json"] }
tempdir = "0.3.7"
thiserror = "1.0.63"
tokio = { version = "1.39.1", features = ["full"] }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No longer requiring async libs

@grod220 grod220 self-assigned this Sep 12, 2024
grod220 and others added 5 commits September 12, 2024 20:46
Applies yamllint reccos and updates to latest version of the 'checkout'
action.
Moves out of the Rust "src/" dir because they're not source code files.
Updates references accordingly.
@conorsch
Copy link
Contributor

Tacked on a few nice-to-haves: yaml linting, more logging, and 🚨 moved the submodule from tools/compiler/src/chain-registry/ to tools/compiler/files/chain-registry/. That last one will require a git submodule sync, but then we should be over the hump of migrating to the new setup.

Copy link
Contributor

@conorsch conorsch left a comment

Choose a reason for hiding this comment

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

Long overdue, very usable, thank you!

@conorsch conorsch merged commit 673ee58 into main Sep 12, 2024
7 checks passed
@conorsch conorsch deleted the submodule-conversion branch September 12, 2024 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

bug: failure to parse AssetTracesItem Restructure inputs and add upstream registry as a submodule
2 participants