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

Clean up ctru-sys version checking #141

Merged
merged 6 commits into from
Nov 27, 2023
Merged

Conversation

ian-h-chamberlain
Copy link
Member

@ian-h-chamberlain ian-h-chamberlain commented Oct 29, 2023

Relates to #100 but maybe not totally resolved by this PR

  • Don't issue a warning for "mismatched version" since the new bindings make this irrelevant

  • Minor build script refactoring and code cleanup

  • Output a release version (-X) for libctru env vars

  • Revert version to 0.5.0, which is higher than we were using before the major 22.x version but should be ok since we never released to crates.io

  • Unconditionally link ctru (not ctrud) due to Is it necessary to modify RUSTFLAGS in cargo-3ds?  cargo-3ds#14

    • don't love this, maybe there's a better option
  • TODO I should update the README since it was documenting the version number convention but that also doesn't apply anymore

- Don't issue a warning for "mismatched version" since the new bindings
  make this irrelevant
- Minor build script refactoring and code cleanup
- Output a `release` version (-X) for libctru env vars
- Revert version to 0.5.0, which is higher than we were using before the
  major 22.x version but should be ok since we never released to crates.io
Also add a little extra description helper for unknown error types in
ctru::error
Since the linker issues are fixed by previous changes we should be able
to run CI without `--package` and test the whole workspace instead.
@Meziu
Copy link
Member

Meziu commented Oct 29, 2023

Nice PR. As i said regarding the bindings PR, I am convinced that ctru-sys's version number should be separate from the libctru version that it links. I've also seen some changes regarding the general structure of tests and other things. I'll look around your changes once I manage to find some time 😅.

@ian-h-chamberlain
Copy link
Member Author

Nice PR. As i said regarding the bindings PR, I am convinced that ctru-sys's version number should be separate from the libctru version that it links. I've also seen some changes regarding the general structure of tests and other things. I'll look around your changes once I manage to find some time 😅.

No rush to review! With the way we're generating the bindings now I agreed in terms of version numbering, which is why I bumped it back down to 0.5.0. Most of the other stuff was just adding build script warnings and refactoring but feel free to review at your own pace 👍

Copy link
Member

@Meziu Meziu left a comment

Choose a reason for hiding this comment

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

Great, the changes were as I expected them to be. The only "issue" would be finding a way to link ctrud correctly, but for the sake of this PR it might not be necessary. Merge if you want.

@ian-h-chamberlain
Copy link
Member Author

I was able to figure out something that I think works a little better for linking ctru, which in combination with rust3ds/cargo-3ds#47 I think makes for a much better solution. Let me know what you think!

`rustc-env` only works for the current crate, but plain keys work for
dependent crates via the `DEP_CTRU` keys, which is probably more useful in
general.
@Meziu Meziu merged commit 727d594 into master Nov 27, 2023
4 checks passed
@Meziu Meziu deleted the fix/cleanup-sys-version-checking branch November 27, 2023 15:35
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