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

External dependency directory #236

Merged
merged 3 commits into from
May 31, 2024
Merged

External dependency directory #236

merged 3 commits into from
May 31, 2024

Conversation

rooooooooob
Copy link
Collaborator

@rooooooooob rooooooooob commented May 29, 2024

Any inputs in a /_CDDL_CODEGEN_EXTERN_DEPS_DIR_/ directory at the root of the --input will now be treated as existing in an external crate.

All types will be ignored for exporting but will still be used in the intermediate steps to determine what exists, what CBOR types they are, etc.

This saves users from having to manually replace all crate::foo::etc and be able to directly have it use foo::etc when they put their foo folder in the aforementioned external deps folder.

This is very useful e.g. for CML's multi-era crate which refers to types from cml-chain, etc. Before this we needed to manually edit imports, manually remove module declarations, and manually delete those files/folders corresponding to the external deps.

A test was added which also tests --common-import-override and integration with an external crate in general.

Also fixes the cddl dependency to specifically =0.9.1 to get around anweiss/cddl#222

Any inputs in a `/_CDDL_CODEGEN_EXTERN_DEPS_DIR_/` directory at the root
of the `--input` will now be treated as existing in an external crate.

All types will be ignored for exporting but will still be used in the
intermediate steps to determine what exists, what CBOR types they are,
etc.

This saves users from having to manually replace all `crate::foo::etc` and be
able to directly have it use `foo::etc` when they put their `foo` folder
in the aforementioned external deps folder.

This is very useful e.g. for CML's multi-era crate which refers to types
from `cml-chain`, etc. Before this we needed to manually edit imports,
manually remove module declarations, and manually delete those
files/folders corresponding to the external deps.
@@ -23,6 +23,7 @@ enum ControlOperator {
}

pub const SCOPE_MARKER: &str = "_CDDL_CODEGEN_SCOPE_MARKER_";
pub const EXTERN_DEPS_DIR: &str = "_CDDL_CODEGEN_EXTERN_DEPS_DIR_";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we not easily have a test for this one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess we could but we'd have to pull in a dependency. I guess we could have a simple crate somewhere in the tests folder and use that and add it to the Cargo.toml of the test after generation as a local dependency. I'll try doing that.

Copy link
Collaborator Author

@rooooooooob rooooooooob May 31, 2024

Choose a reason for hiding this comment

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

I added a test. It was a bit more work than I expected. It ended up also testing --common-import-override necessarily so that's a plus at least. I also fixed the failing tests (was only happening on github). Turns out after running cargo from a clean install would cause cddl to update to 0.9.4 (locally it was still using 0.9.1. as specified for me still) which broke basic group support parsing, at least in one of the ways it was used in some of the tests. I made an issue for it anweiss/cddl#222 and set it to =0.9.1 to get around that.

@rooooooooob rooooooooob merged commit e75a0ad into master May 31, 2024
1 check passed
@SebastienGllmt SebastienGllmt deleted the extern-deps-dir branch May 31, 2024 20:59
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.

2 participants