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

Version 1.4.0 rc updates to bazel and protobuf comments #499

Merged
merged 3 commits into from
Sep 16, 2024

Conversation

chrispsommers
Copy link
Collaborator

@chrispsommers chrispsommers commented Sep 13, 2024

Merge accumulated 1.4.0 updates by consensus in WG mtg:

  • Add missing comments "Added in 1.4.0" and "Deprecated in 1.4.0" to protobufs
  • Update script bazel comments to reflect version 1.4.0
  • Regenerate protobuf client code

@smolkaj
Copy link
Member

smolkaj commented Sep 13, 2024

FYI, started a "prerelease" for 1.4.0 so the URLs in the Bazel rules work: https://github.com/p4lang/p4runtime/releases/tag/v1.4.0

@antoninbas
Copy link
Member

FYI, started a "prerelease" for 1.4.0 so the URLs in the Bazel rules work: https://github.com/p4lang/p4runtime/releases/tag/v1.4.0

@smolkaj Once you create a tag (in this case v1.4.0) in the Git upstream, you are never supposed to mutate that tag (i.e., mutate the commit referenced by that tag, which is currently f50fef9). That's pretty much a golden rule, and it can break things if you do (e.g., Go modules - it's very much a pain to repair properly and can only be done by releasing a new greater version). Did you mean to create that tag already? Marking the Github release as a "pre-release" doesn't make a difference here, because the tag is still created the same way.

If f50fef9 was the intended commit for the v1.4.0 tag all along, please ignore, but it doesn't seem to be the case, given the existence of this PR. The only way to fix it without breaking anything would be to leave the v1.4.0 tag alone now, and release v1.4.1 with all the necessary extra changes.

@smolkaj
Copy link
Member

smolkaj commented Sep 13, 2024

Thanks for catching that. IIUC, you are saying that tags should always be immutable?

Would simply deleting the tag again address your concerns? Presumably no one has started using it quite yet :)

@antoninbas
Copy link
Member

Thanks for catching that. IIUC, you are saying that tags should always be immutable?

Would simply deleting the tag again address your concerns? Presumably no one has started using it quite yet :)

No deleting the tag doesn't work.
The tag would already have been cache by https://proxy.golang.org/ (presumably right after you created it). See how it is already in the Go documentation: https://pkg.go.dev/github.com/p4lang/p4runtime?tab=versions
Mutating the tag, deleting the tag, deleting the tag in order to recreate it later (which is essentially the same as mutating it) can all create issues for downstream modules (again in the case of Golang) because of conflicts with https://sum.golang.org/
Even outside of Golang specifically, mutating a tag can create issues for software that depends on the project. Some processes which observe a tag that has changed, or release assets that have changed, can fail as it gets reported as a supply chain attack.
The only solution is to move on to v1.4.1 without changing v1.4.0.

@smolkaj
Copy link
Member

smolkaj commented Sep 13, 2024

Thanks for elaborating, Antonin, I was not aware of that.
SG regarding moving to v1.4.1.

chrispsommers and others added 2 commits September 16, 2024 10:32
* Added missing "Added/deprecated in v1.4.0" comments per convention.

* Added missing "Added/deprecated in v1.4.0" comments per convention.

* Refresh generated go files.

Signed-off-by: chris <chris.sommers@keysight.com>
Signed-off-by: Steffen Smolka <smolkaj@google.com>
Signed-off-by: chris <chris.sommers@keysight.com>
Signed-off-by: chris <chris.sommers@keysight.com>
@chrispsommers chrispsommers merged commit dda9d66 into p4lang:main Sep 16, 2024
5 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.

4 participants