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

chore(bin): rename mm2 binaries to kdf #2126

Merged
merged 7 commits into from
Jul 3, 2024
Merged

chore(bin): rename mm2 binaries to kdf #2126

merged 7 commits into from
Jul 3, 2024

Conversation

shamardy
Copy link
Collaborator

This PR renames mm2 binaries and libs to kdf

@shamardy shamardy added the in progress Changes will be made from the author label May 24, 2024
@laruh
Copy link
Member

laruh commented May 24, 2024

Should these paths also be renamed?

Screenshot 2024-05-24 at 19 19 01

@shamardy
Copy link
Collaborator Author

Should these paths also be renamed?

Thanks for finding these, I am not sure they are used but will rename them.

@borngraced
Copy link
Member

What about renaming mm2_* crates name too?

@laruh
Copy link
Member

laruh commented May 24, 2024

What about renaming mm2_* crates name too?

I think it will be a rabbit hole for this pr :D wallet team asked for the binaries at least. But its up to Omer

@shamardy
Copy link
Collaborator Author

What about renaming mm2_* crates name too?

We will do the renaming gradually whenever a need for renaming arises, this way we don't create much conflicts in current open PRs.

@shamardy shamardy added under review and removed in progress Changes will be made from the author labels May 24, 2024
@shamardy shamardy requested review from onur-ozkan and laruh May 24, 2024 12:47
Copy link
Member

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

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

Making this change will break many deployment setups. We should make sure people (specially notary node and our devops people) aware that we are going to rename the binary.

@onur-ozkan onur-ozkan added the breaking-change might require breaking backward compatibility label May 24, 2024
@shamardy
Copy link
Collaborator Author

Making this change will break many deployment setups. We should make sure people (specially notary node and our devops people) aware that we are going to rename the binary.

Already informed @KomodoPlatform/qa to update docs and they will be responsible for informing other people. I will include the breaking changes in the release notes too, should we change next release to v3.0.0-beta to follow semantic versioning?

@laruh
Copy link
Member

laruh commented May 24, 2024

should we change next release to v3.0.0-beta to follow semantic versioning

yes, def should do it. as its breaking change and its not stable, then 3 0 0 beta version should be

@onur-ozkan
Copy link
Member

Providing a copy with mm2 name next to kdf binary would be fine for a while (~3 months). I don't think we should bump the major version just because we renamed the binary (it's breaking change for deployments not the application logic).

@laruh
Copy link
Member

laruh commented May 24, 2024

https://semver.org/#spec-item-8

Major version X (X.y.z | X > 0) MUST be incremented if any backward incompatible changes are introduced to the public API. It MAY also include minor and patch level changes. Patch and minor versions MUST be reset to 0 when major version is incremented.

additional source
https://www.infoq.com/articles/breaking-changes-are-broken-semver/

As I see any breaking changes that can cause existing systems or scripts to fail (changes in binary names, config parameters, or deployment paths, as these can disrupt automated processes and user workflows etc) should be treated as Major version changes.

@onur-ozkan
Copy link
Member

https://semver.org/#spec-item-8

Major version X (X.y.z | X > 0) MUST be incremented if any backward incompatible changes are introduced to the public API. It MAY also include minor and patch level changes. Patch and minor versions MUST be reset to 0 when major version is incremented.

additional source https://www.infoq.com/articles/breaking-changes-are-broken-semver/

As I see any breaking changes that can cause existing systems or scripts to fail (changes in binary names, config parameters, or deployment paths, as these can disrupt automated processes and user workflows etc) should be treated as Major version changes.

We don't change the API that's the point. With major bump, you want to inform people that you made a breaking change, right? How are we supposed to get the version information? The binary name has been changed :) You version the application, here we don't change the application. As I said, this is breaking change for deployments not application itself.

@shamardy
Copy link
Collaborator Author

Providing a copy with mm2 name next to kdf binary would be fine for a while (~3 months)

This is for the release artifacts, right? we don't need to do this in CI, or did you mean that?

@shamardy
Copy link
Collaborator Author

As I see any breaking changes that can cause existing systems or scripts to fail (changes in binary names, config parameters, or deployment paths, as these can disrupt automated processes and user workflows etc) should be treated as Major version changes.

That's correct, but I am not sure we need this currently while we are beta, the first stable release will start at 1.0.0 again and then we will need to make sure semver is right with every new release. Usually for our beta version if we made a big change like the p2p network upgrade we would bump the major version indicating a big change otherwise we just bump the minor version.

Comment on lines 24 to 29
[[bin]]
name = "mm2"
name = "kdf"
path = "src/mm2_bin.rs"
test = false
doctest = false
bench = false
Copy link
Member

@onur-ozkan onur-ozkan May 24, 2024

Choose a reason for hiding this comment

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

Providing a copy with mm2 name next to kdf binary would be fine for a while (~3 months)

This is for the release artifacts, right? we don't need to do this in CI, or did you mean that?

For anything (release, CI builds, containers, etc). If you duplicate this [[bin]] block with old name, cargo should produce 2 binaries (mm2 and kdf). I think we don't spend too much time on building the final binary output, so build time should not increase too much.

Copy link
Collaborator Author

@shamardy shamardy May 24, 2024

Choose a reason for hiding this comment

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

cargo should produce 2 binaries (mm2 and kdf)

It does that, but I get a warning

warning: /atomicDEX-API/mm2src/mm2_bin_lib/Cargo.toml: file found to be present in multiple build targets: /atomicDEX-API/mm2src/mm2_bin_lib/src/mm2_bin.rs

I will leave it. I can't duplicate the [lib] block so I will leave it as kdflib only. Will work on CI and others now.

Copy link
Collaborator Author

@shamardy shamardy May 24, 2024

Choose a reason for hiding this comment

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

For windows, I renamed the exe only to mm2 as the dll should remain kdflib based on the lib name. For wasm, I don't think we can have both kdflib_bg.wasm and mm2lib_bg.wasm without having 2 build crates so I decided to have only one with kdf prefix. For all other targets, we now have 2 binaries.

@shamardy shamardy added in progress Changes will be made from the author and removed under review labels May 24, 2024
@shamardy shamardy added under review and removed in progress Changes will be made from the author labels May 24, 2024
@laruh
Copy link
Member

laruh commented May 26, 2024

As I see any breaking changes that can cause existing systems or scripts to fail (changes in binary names, config parameters, or deployment paths, as these can disrupt automated processes and user workflows etc) should be treated as Major version changes.

That's correct, but I am not sure we need this currently while we are beta, the first stable release will start at 1.0.0 again and then we will need to make sure semver is right with every new release. Usually for our beta version if we made a big change like the p2p network upgrade we would bump the major version indicating a big change otherwise we just bump the minor version.

So, if I understand you correctly, we will strictly follow the semver rules starting with the "stable" version, and our "beta" allows for some flexibility. This approach is fine with me, since we will be moving to the stable version soon. I don't insist on the current version as long as we have clear rules for the stable releases.

Copy link
Member

@laruh laruh left a comment

Choose a reason for hiding this comment

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

one question

.docker/Dockerfile.release Outdated Show resolved Hide resolved
WORKDIR /mm2
COPY target/release/mm2 /usr/local/bin/mm2
WORKDIR /kdf
COPY target/release/kdf /usr/local/bin/kdf
Copy link
Member

Choose a reason for hiding this comment

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

Let's ship mm2 binary into containers as well:

Suggested change
COPY target/release/kdf /usr/local/bin/kdf
COPY target/release/kdf /usr/local/bin/kdf
COPY target/release/mm2 /usr/local/bin/mm2

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@laruh
Copy link
Member

laruh commented May 28, 2024

a1e288c Now we have two Dockerfiles with identical commands
Screenshot 2024-05-28 at 19 30 23

But they are used in diff build.yml 🤔
Screenshot 2024-05-28 at 19 36 55

@shamardy
Copy link
Collaborator Author

a1e288c Now we have two Dockerfiles with identical commands

Let's leave them in case we ever need different commands for dev or release.

Copy link
Member

@laruh laruh left a comment

Choose a reason for hiding this comment

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

LGTM!

@shamardy
Copy link
Collaborator Author

shamardy commented Jul 2, 2024

@onur-ozkan can you please give this PR another/last review? I will inform the GUI team about the changes before merging so that they are ready.

Copy link
Member

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

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

LGTM

cc @smk762 (you may want to be aware of this merge.)

@shamardy shamardy merged commit bc8757a into dev Jul 3, 2024
21 of 25 checks passed
@shamardy shamardy deleted the rename-mm2-bin-kdf branch July 3, 2024 19:49
dimxy added a commit that referenced this pull request Jul 21, 2024
* dev:
  feat(nft-swap): add standalone maker contract and proxy support (#2100)
  feat(ETH): add `gas_limit` coins param to override default values (#2137)
  feat(tendermint): implement better sequence resolving logic (#2164)
  ci(artifact): add target for macos on apple silicon (#2163)
  fix(helpers): extend http to ws address conversion (#2166)
  fix(makerbot): add "testcoin" to provider options (#2161)
  fix(hd_wallet): make extended pubkey of hd wallet generic (#2159)
  fix(docker-tests): implement containers runtime directories (#2162)
  feat(tendermint): improve the `max` handling for tendermint withdraw (#2155)
  revert #2158 (comment) (#2160)
  ci(artifacts): upload build artifacts with in-tree script (#2158)
  test(tendermint): migrate to local/offline containerized testnets (#2128)
  use easingthemes/ssh-deploy@v5.0.3 for all builds except windows (#2157)
  chore(bin): rename mm2 binaries to kdf (#2126)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change might require breaking backward compatibility under review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants