Skip to content

cbindgen: Add version defines #576

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

Merged
merged 2 commits into from
Jul 10, 2025
Merged

cbindgen: Add version defines #576

merged 2 commits into from
Jul 10, 2025

Conversation

yedayak
Copy link
Contributor

@yedayak yedayak commented Jun 24, 2025

This will allow users to check the version of rustls-ffi without resorting to checking if functions are defined.

In addition to RUSTLS_VERSION_{MAJOR,MINOR,PATCH}, also define RUSTLS_VERSION_NUMBER, which includes each version part in it, bit shifted to the left. This is inspired by openssl[0], c-ares[1]. There are other options for this, for example zstd multiplies each part by a power of 100[2].

We might want to also have the entire version string here, which could help tools that need to parse the header file itself.

[0] https://github.com/openssl/openssl/blob/cdd01b5e0734b0324251b32a8edd97f42ba90429/include/openssl/opensslv.h.in#L92-L102
[1] https://github.com/c-ares/c-ares/blob/42ddbc14ec008e738fa44aa2c16e74cad93742c2/include/ares_version.h#L43-L45
[2] https://github.com/facebook/zstd/blob/3c3b8274c517727952927c705940eb90c10c736f/lib/zstd.h#L115

Fixes #557

@cpu
Copy link
Member

cpu commented Jun 24, 2025

Thanks for picking this up! Do you think it's possible to extend the rustls version test to enforce keeping this in sync?

@cpu
Copy link
Member

cpu commented Jun 25, 2025

Do you think it's possible to extend the rustls version test to enforce keeping this in sync?

There are some pieces for parsing .h files in docgen/main.rs that might also be helpful for this job.

Writing the test is likely to be more work than it was to implement the change itself so I'm also OK leaving that as follow-up work if you're not keen on tackling it yourself.

@yedayak
Copy link
Contributor Author

yedayak commented Jun 30, 2025

I made the test also verify the defines in the header file

@yedayak
Copy link
Contributor Author

yedayak commented Jun 30, 2025

I'm not sure what to do about the failures in rust 1.73, tree-sitter-language requires rust 1.76.

@cpu
Copy link
Member

cpu commented Jul 1, 2025

I'm not sure what to do about the failures in rust 1.73, tree-sitter-language requires rust 1.76.

Ahh crud. I forgot that was one of the motivators for having a separate tools crate for the doc generation tooling.

The simplest thing to do might be to move the whole test to tools. As long as it's run in CI I think that's probably just as good as having it in librustls. WDYT?

@yedayak
Copy link
Contributor Author

yedayak commented Jul 8, 2025

I'm not sure what to do about the failures in rust 1.73, tree-sitter-language requires rust 1.76.

Ahh crud. I forgot that was one of the motivators for having a separate tools crate for the doc generation tooling.

The simplest thing to do might be to move the whole test to tools. As long as it's run in CI I think that's probably just as good as having it in librustls. WDYT?

I moved just the new part to tools, since the current test is dependent on the features of librustls and I couldn't find a good way to use them from tools.
About running it in CI, I'm not sure what exactly is needed to do that, does it need to be added to some cmake target or something?

@yedayak yedayak force-pushed the define-headers branch 2 times, most recently from af1e8dd to f403ba5 Compare July 8, 2025 20:29
@cpu cpu self-requested a review July 9, 2025 16:42
@cpu
Copy link
Member

cpu commented Jul 9, 2025

I moved just the new part to tools, since the current test is dependent on the features of librustls and I couldn't find a good way to use them from tools.

SGTM. Thanks!

About running it in CI, I'm not sure what exactly is needed to do that, does it need to be added to some cmake target or something?

The cmake stuff is rooted inlibrustls so it won't be a good fit for this.

I think the best route here is to add a new job block (maybe after the minver job block?) that runs the tests from the non-default rustls-ffi-tools workspace member. Something like:

tools:
  name: Test rustls-ffi-tools
  runs-on: ubuntu-latest
  steps:
    - name: Checkout sources
      uses: actions/checkout@v4
      with:
        persist-credentials: false

    - name: Install rust toolchain
      uses: dtolnay/rust-toolchain@nightly
        
    - name: Run tools unit tests
      run: cargo test -p rustls-ffi-tools

We don't want to add the cargo test step to the main build job because it would have to be skipped for the MSRV. We could do it as part of ensure-header-updated but it seems clearer to just have a new job. WDYT?

Copy link
Member

@cpu cpu left a comment

Choose a reason for hiding this comment

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

Looks great! I had a last round of nits but assuming those are addressed and nobody else has feedback I think this is ready for merge.

I appreciate you putting in the extra work to land the test. I know it was a lot more effort than the actual .h update 😅

yedayak added 2 commits July 10, 2025 21:08
This will allow users to check the version of rustls-ffi without
resorting to checking if functions are defined.

Also add make rustls_version_match verify that defines are the correct.

In addition to RUSTLS_VERSION_{MAJOR,MINOR,PATCH}, also define
RUSTLS_VERSION_NUMBER, which includes each version part in it, bit shifted to the
left. This is inspired by openssl[0], c-ares[1]. There are other options for
this, for example zstd multiplies each part by a power of 100[2].

We might want to also have the entire version string here, which could
help tools that need to parse the header file itself.

[0] https://github.com/openssl/openssl/blob/cdd01b5e0734b0324251b32a8edd97f42ba90429/include/openssl/opensslv.h.in#L92-L102
[1] https://github.com/c-ares/c-ares/blob/42ddbc14ec008e738fa44aa2c16e74cad93742c2/include/ares_version.h#L43-L45
[2] https://github.com/facebook/zstd/blob/3c3b8274c517727952927c705940eb90c10c736f/lib/zstd.h#L115

Fixes rustls#557
@cpu cpu merged commit ef4e484 into rustls:main Jul 10, 2025
47 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.

Include rustls-ffi version parts in rustls.h header
2 participants