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

Fix UUID for Darwin hosts #103

Merged
merged 7 commits into from
Aug 23, 2024
Merged

Fix UUID for Darwin hosts #103

merged 7 commits into from
Aug 23, 2024

Conversation

dcarbone
Copy link
Contributor

@dcarbone dcarbone commented Aug 2, 2024

- Addresses rs#100 based on fix suggested by @rpendleton
- Adds multi-platform matrix run strategy to actions
- Adds .gitignore file
@dcarbone
Copy link
Contributor Author

dcarbone commented Aug 2, 2024

Should be noted that I do not have a Darwin machine handy to test this on, thus I'm relying on gha runners :D

@rs
Copy link
Owner

rs commented Aug 5, 2024

Thanks for the PR. Could you please check the failing tests?

@dcarbone
Copy link
Contributor Author

dcarbone commented Aug 8, 2024

Sorry for the slow response! Bumped to go1.16, which seems to be the oldest version with an available darwin arm64 build.

@rs
Copy link
Owner

rs commented Aug 8, 2024

Thanks. It is still failing due to some wrong build tags apparently.

@dcarbone
Copy link
Contributor Author

dcarbone commented Aug 9, 2024

@rs Reverted that line back to the way it was. I'm used to the go1.17+ build tags, it seems :)

@rs
Copy link
Owner

rs commented Aug 9, 2024

Damn there is now a lint issue on windows. Do you mind applying the recommendation to make it pass? Thanks!

- Explicitly ignoring defer in registry key closure
- Using syscall.UTF16PtrFromString in lieu of deprecated StringToUTF16Ptr func
@dcarbone
Copy link
Contributor Author

@rs Sorry again for the delay! Windows lint issues addressed :)

@dcarbone
Copy link
Contributor Author

@rs Sorry to bug you, but I believe the last of the issues have been adressed :)

@rs rs merged commit 7aed5a8 into rs:master Aug 23, 2024
8 checks passed
@dcarbone
Copy link
Contributor Author

A thought: does this constitute a major version release, given the change in id generation? Presumably 3rd party impls will also have to update.

@rs
Copy link
Owner

rs commented Aug 23, 2024

There is no change of API or anything non-backward compatible, so it should remain a minor. Do I miss something?

@dcarbone
Copy link
Contributor Author

dcarbone commented Aug 23, 2024

A key value source is different for Darwin now, no? I don't have a Darwin host to compare & contrast, unfortunately...

Edit: ah, but we don't actually care, do we. Its uniqueness and stability is important, not the source.

@rs
Copy link
Owner

rs commented Aug 23, 2024

How is that affecting compatibility?

@dcarbone
Copy link
Contributor Author

If they generate a key on the same host using a lib with the previous mechanism, it will not match a value created with this change.

@rs
Copy link
Owner

rs commented Aug 23, 2024

What do you mean by "match"? The lib is meant to generate unique ids, two values generated by the lib should never match regardless of the lib version :)

@dcarbone dcarbone deleted the dcarbone/darwin-uuid branch August 23, 2024 23:18
@dcarbone
Copy link
Contributor Author

Ah, but the individual components are comparable, too, no?

@rs
Copy link
Owner

rs commented Aug 24, 2024

There is no guarantee the machine id would stay stable across versions.

@dcarbone
Copy link
Contributor Author

Fair enough 😄

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