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

Revert to passing BinOutputPath as standalone field in go build #385

Merged
merged 3 commits into from
Apr 11, 2024

Conversation

wojtek-coreum
Copy link
Collaborator

@wojtek-coreum wojtek-coreum commented Apr 9, 2024

Description

The original way was there for purpose. If cmd.Dir is not set, go.mod for the main module is used by the build command instead of the one defined in the submodule.

Reviewers checklist:

  • Try to write more meaningful comments with clear actions to be taken.
  • Nit-picking should be unblocking. Focus on core issues.

Authors checklist

  • Provide a concise and meaningful description
  • Review the code yourself first, before making the PR.
  • Annotate your PR in places that require explanation.
  • Think and try to split the PR to smaller PR if it is big.

This change is Reviewable

@wojtek-coreum wojtek-coreum requested a review from a team as a code owner April 9, 2024 10:00
@wojtek-coreum wojtek-coreum requested review from masihyeganeh, dzmitryhil, miladz68 and ysv and removed request for a team April 9, 2024 10:00
Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @masihyeganeh, @miladz68, and @wojtek-coreum)


build/golang/go.go line 99 at r1 (raw file):

	cmd := exec.Command(tools.Path("bin/go", tools.TargetPlatformLocal), args...)
	cmd.Dir = config.PackagePath

this doesn't work properly, that is why I changed it

Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @masihyeganeh, @miladz68, and @wojtek-coreum)


build/golang/go.go line 99 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

this doesn't work properly, that is why I changed it

why do we need this ?
What are the benefits of this approach ?

Copy link
Collaborator Author

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @masihyeganeh, @miladz68, and @ysv)


build/golang/go.go line 99 at r1 (raw file):

this doesn't work properly, that is why I changed it

What didn't work?

What are the benefits of this approach ?

This is how go build works. To use the correct go.mod you need to be in the right directory.
Try to run bin/bdjuno-builder build/me in the bdjuno repo.

Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @masihyeganeh, @miladz68, and @wojtek-coreum)


build/golang/go.go line 99 at r1 (raw file):

What didn't work?

Test it in coreum repo with native build & in docker.
As far as I remember when building in docker it didn't work properly. Since it changes directory, so resulting binary was mounted to build/cmd/bin/.cache/cored/docker.linux.arm64/bin/cored instead of bin/.cache/cored/docker.linux.arm64/bin/cored. Smth like this

Copy link
Collaborator Author

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @masihyeganeh, @miladz68, and @ysv)


build/golang/go.go line 99 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

What didn't work?

Test it in coreum repo with native build & in docker.
As far as I remember when building in docker it didn't work properly. Since it changes directory, so resulting binary was mounted to build/cmd/bin/.cache/cored/docker.linux.arm64/bin/cored instead of bin/.cache/cored/docker.linux.arm64/bin/cored. Smth like this

I see path is correct

miladz68
miladz68 previously approved these changes Apr 9, 2024
Copy link
Contributor

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @masihyeganeh, and @ysv)

dzmitryhil
dzmitryhil previously approved these changes Apr 9, 2024
Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @masihyeganeh and @ysv)

Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @masihyeganeh and @wojtek-coreum)


build/golang/go.go line 99 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

I see path is correct

workDir = "/build/cmd"

-o = "bin/.cache/..."

check:
go build -o="bin/.cache"

Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @masihyeganeh and @wojtek-coreum)


build/golang/go.go line 99 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

workDir = "/build/cmd"

-o = "bin/.cache/..."

check:
go build -o="bin/.cache"

Discussed issue on call

@wojtek-coreum wojtek-coreum dismissed stale reviews from dzmitryhil and miladz68 via 27329a1 April 10, 2024 11:13
Copy link
Collaborator Author

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @masihyeganeh, @miladz68, and @ysv)


build/golang/go.go line 99 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

Discussed issue on call

Done.

@dzmitryhil dzmitryhil requested review from miladz68 and ysv April 10, 2024 11:26
Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @masihyeganeh, @miladz68, and @ysv)

Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @masihyeganeh and @miladz68)

@ysv ysv changed the title Revert to setting cmd.Dir in go commands Revert to passing BinOutputPath as standalone field in go build Apr 11, 2024
@ysv ysv merged commit e007fb9 into master Apr 11, 2024
3 checks passed
@ysv ysv deleted the wojtek/fix-build-dir branch April 11, 2024 14:14
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.

None yet

4 participants