Skip to content

Conversation

@JurajSadel
Copy link
Contributor

This might be a bit of a hacky solution, but I tried to convince cargo-semver-checks that esp-rom-sys is not intended to ever reach 1.0, and I failed.

To test this, I changned a pub fn name in esp-rom-sys/src/rom/crc.rs:

-pub fn crc8_be(crc: u8, buf: &[u8]) -> u8 {
+pub fn crc8_be_change(crc: u8, buf: &[u8]) -> u8 {

Ran cargo xrelease plan esp-rom-sys --allow-non-main:

[2025-12-01T14:51:54Z INFO  xtask::commands::release::semver_check::checker] Downloading API baselines for esp-rom-sys
[2025-12-01T14:51:54Z INFO  xtask::commands::release::semver_check::checker] Attempting to download from GitHub Actions artifact: api-baselines-esp-rom-sys
[2025-12-01T14:51:54Z INFO  xtask::commands::release::semver_check::checker] Attempting to download artifact: api-baselines-esp-rom-sys from esp-rs/esp-hal
[2025-12-01T14:51:56Z INFO  xtask::commands::release::semver_check::checker] Successfully downloaded baselines from artifact api-baselines-esp-rom-sys
[2025-12-01T14:51:56Z INFO  xtask::commands::release::semver_check::checker] Found 7 baseline files for esp-rom-sys
    Checking <unknown> v0.1.3 -> v0.1.3 (no change; assume minor)
     Checked [   0.024s] 140 checks: 139 pass, 1 fail, 0 warn, 38 skip

--- failure function_missing: pub fn removed or renamed ---

Description:
A publicly-visible function cannot be imported by its prior path. A `pub use` may have been removed, or the function itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.43.0/src/lints/function_missing.ron

Failed in:
  function esp_rom_sys::rom::crc::crc8_be, previously in file src/rom/crc.rs:122

     Summary semver requires new major version: 1 major and 0 minor checks failed

Summary semver requires new major version: 1 major and 0 minor checks failed

Is the reason for the "hacky" solution.

The final .jsonc:

{
  "base": "esp-rom-sys-semver",
  "packages": [
    {
      "package": "esp-metadata-generated",
      "semver_checked": false,
      "current_version": "0.3.0",
      "new_version": "0.4.0",
      "tag_name": "esp-metadata-generated-v0.4.0",
      "bump": "Minor"
    },
    {
      "package": "esp-rom-sys",
      "semver_checked": true,
      "current_version": "0.1.3",
      "new_version": "0.2.0",
      "tag_name": "esp-rom-sys-v0.2.0",
      "bump": "Minor"
    }
  ]
}

closes #4489

@JurajSadel JurajSadel added the skip-changelog No changelog modification needed label Dec 1, 2025
@MabezDev
Copy link
Member

MabezDev commented Dec 1, 2025

FYI, there is cargo xcheck semver-check check which will download the baselines and test against the current local code. Would be good to test that this works with more than just esp-hal.

@bugadani
Copy link
Contributor

bugadani commented Dec 1, 2025

I don't mind doing this, but it's worth mentioning that what the release tool considers Major/Minor/Patch don't actually match semver's idea of "0.x -> 0.x+1 is a major bump".

@JurajSadel
Copy link
Contributor Author

FYI, there is cargo xcheck semver-check check which will download the baselines and test against the current local code. Would be good to test that this works with more than just esp-hal.

I tested it (with the same change in esp-rom-sys as in the description) and the result is the same for esp-rom-sys. How esp-hal:

Finished `dev` profile [unoptimized + debuginfo] target(s) in 1.01s
   Generated /Users/jurajsadel/esp-rs/esp-hal/target/xtensa-esp32-none-elf/doc/esp_hal.json
    Checking <unknown> v1.0.0-rc.0 -> v1.0.0 (major change)
     Checked [   0.000s] 0 checks: 0 pass, 178 skip
     Summary no semver update required
    Finished [   0.204s] <unknown>

So, we need to re-generate the baselines for esp-hal and I changed impl<'d> Uart<'d, Blocking>

-    pub fn new(uart: impl Instance + 'd, config: Config) -> Result<Self, ConfigError> {
+    pub fn new1(uart: impl Instance + 'd, config: Config) -> Result<Self, ConfigError> {

and ran cargo xcheck semver-check check:

Finished `dev` profile [unoptimized + debuginfo] target(s) in 1.01s
   Generated /Users/jurajsadel/esp-rs/esp-hal/target/xtensa-esp32-none-elf/doc/esp_hal.json
    Checking <unknown> v1.0.0 -> v1.0.0 (no change; assume patch)
     Checked [   0.081s] 165 checks: 164 pass, 1 fail, 0 warn, 13 skip

--- failure inherent_method_missing: pub method removed or renamed ---
Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.15s
   Generated /Users/jurajsadel/esp-rs/esp-hal/target/xtensa-esp32s3-none-elf/doc/esp_rom_sys.json
    Checking <unknown> v0.1.3 -> v0.1.3 (no change; assume minor)
     Checked [   0.023s] 140 checks: 139 pass, 1 fail, 0 warn, 38 skip

--- failure function_missing: pub fn removed or renamed ---

So, it should be working correctly (with re-generated baseline for esp-hal).

@JurajSadel
Copy link
Contributor Author

Is anything else needed here, please?

}
}

let forever_unstable = package
Copy link
Contributor

Choose a reason for hiding this comment

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

not necessarily something which needs to be done in this PR but we could have a forever_unstable() method on Package - we now have this piece of code duplicated

Copy link
Member

Choose a reason for hiding this comment

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

imo this should be done here, as its the first copy of the original. Let's refactor this now.

@MabezDev
Copy link
Member

MabezDev commented Dec 3, 2025

FYI, there is cargo xcheck semver-check check which will download the baselines and test against the current local code. Would be good to test that this works with more than just esp-hal.

I tested it (with the same change in esp-rom-sys as in the description) and the result is the same for esp-rom-sys. How esp-hal:

Finished `dev` profile [unoptimized + debuginfo] target(s) in 1.01s
   Generated /Users/jurajsadel/esp-rs/esp-hal/target/xtensa-esp32-none-elf/doc/esp_hal.json
    Checking <unknown> v1.0.0-rc.0 -> v1.0.0 (major change)
     Checked [   0.000s] 0 checks: 0 pass, 178 skip
     Summary no semver update required
    Finished [   0.204s] <unknown>

So, we need to re-generate the baselines for esp-hal and I changed impl<'d> Uart<'d, Blocking>

-    pub fn new(uart: impl Instance + 'd, config: Config) -> Result<Self, ConfigError> {
+    pub fn new1(uart: impl Instance + 'd, config: Config) -> Result<Self, ConfigError> {

and ran cargo xcheck semver-check check:

Finished `dev` profile [unoptimized + debuginfo] target(s) in 1.01s
   Generated /Users/jurajsadel/esp-rs/esp-hal/target/xtensa-esp32-none-elf/doc/esp_hal.json
    Checking <unknown> v1.0.0 -> v1.0.0 (no change; assume patch)
     Checked [   0.081s] 165 checks: 164 pass, 1 fail, 0 warn, 13 skip

--- failure inherent_method_missing: pub method removed or renamed ---
Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.15s
   Generated /Users/jurajsadel/esp-rs/esp-hal/target/xtensa-esp32s3-none-elf/doc/esp_rom_sys.json
    Checking <unknown> v0.1.3 -> v0.1.3 (no change; assume minor)
     Checked [   0.023s] 140 checks: 139 pass, 1 fail, 0 warn, 38 skip

--- failure function_missing: pub fn removed or renamed ---

So, it should be working correctly (with re-generated baseline for esp-hal).

Can you add a test for this logic within the xtask? This kind of information is forgotten quickly, best to have the test there to enforce that how it behaves now is correct.

Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

I'd like to see a test for the amount bump on packages that are stable and those that are not. Please also address @bjoernQ's comment here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-changelog No changelog modification needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

esp-rom-sys: add semver checking

4 participants