Skip to content

Update Go Ort#3469

Merged
ikolomi merged 43 commits intorelease-2.0from
go/ort-update
Jun 15, 2025
Merged

Update Go Ort#3469
ikolomi merged 43 commits intorelease-2.0from
go/ort-update

Conversation

@prateek-kumar-improving
Copy link
Copy Markdown
Collaborator

@prateek-kumar-improving prateek-kumar-improving commented Mar 31, 2025

Issue link

This Pull Request is linked to issue (URL): #3470

Description

  1. Run GO ORT tool on GO folder. (Using ./.github/workflows/run-ort-tools)
  2. Install Protobuf dependency for running ORT tool.
  3. Update go/.ort.yml to add the files and folders to exclude in ORT for GO.

Changes after review.

  1. Added GoMod to enabledPackageManagers.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one issue.
  • Commit message has a detailed description of what changed and why.
  • Tests are added or updated.
  • CHANGELOG.md and documentation files are updated.
  • Destination branch is correct - main or release
  • Create merge commit if merging release branch into main, squash otherwise.

Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>
@prateek-kumar-improving prateek-kumar-improving requested a review from a team as a code owner March 31, 2025 15:56
@Yury-Fridlyand Yury-Fridlyand added java ☕ issues and fixes related to the java client CI/CD ⚒️ CI/CD related labels Mar 31, 2025
@Yury-Fridlyand
Copy link
Copy Markdown

CI is red. I guess we can't use both Gradle and GradleInspector at a time, or we they require extra config.

@yipin-chen yipin-chen requested a review from barshaul April 4, 2025 19:33
Copy link
Copy Markdown
Collaborator

@yipin-chen yipin-chen left a comment

Choose a reason for hiding this comment

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

Conditional approve, for workflow change to add GoMod
Why do you keep Gradle, GradleInspector, Maven, NPM, etc? Is it due to cross-client dependency?
For Go package and FFI, I will let Yury or Bar to review and comment.

Copy link
Copy Markdown
Collaborator

@barshaul barshaul left a comment

Choose a reason for hiding this comment

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

I don't understand this change. What's the motivation?
Why not to add only the go package manager?
This field should include only the package managers we're publishing to

@prateek-kumar-improving
Copy link
Copy Markdown
Collaborator Author

prateek-kumar-improving commented Apr 25, 2025

I don't understand this change. What's the motivation? Why not to add only the go package manager? This field should include only the package managers we're publishing to

@barshaul
Can you tell how should we get the list of package managers we are publishing to?

@barshaul
Copy link
Copy Markdown
Collaborator

I don't understand this change. What's the motivation? Why not to add only the go package manager? This field should include only the package managers we're publishing to

@barshaul

Can you tell how should we get the list of package managers we are publishing to?

The list you've changed is the updated list of the package managers we're publishing too. Don't override it, instead just add the package manager that is relevant to go. In general- you can see in our CD workflows which package managers are being used

Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>
Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>
Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>
Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>
Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>
Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>
Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>
Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>
@barshaul
Copy link
Copy Markdown
Collaborator

barshaul commented Jun 8, 2025

those are packages used in tests only and they are added to the exclude list:

I initially assumed the first require block in go.mod was for production and the second for dev dependencies, but it appears Go doesn’t make this distinction built-in. So no, test dependencies shouldn’t be included—only protobuf is actually required. However, it’s still missing from the generated NOTICE file. The NOTICE DEFAULT file of Go shouldn't be empty

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
@Yury-Fridlyand Yury-Fridlyand changed the base branch from main to release-2.0 June 10, 2025 22:40
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
@Yury-Fridlyand Yury-Fridlyand marked this pull request as ready for review June 11, 2025 00:55
@ikolomi ikolomi dismissed barshaul’s stale review June 15, 2025 16:24

Approved be delegation

@ikolomi ikolomi merged commit afd1ce7 into release-2.0 Jun 15, 2025
22 of 23 checks passed
@Yury-Fridlyand Yury-Fridlyand deleted the go/ort-update branch June 16, 2025 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI/CD ⚒️ CI/CD related go 🏃 golang wrapper

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants