Skip to content

cmd/go: not print not-defaults arch-env #67492

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

Closed
qiulaidongfeng opened this issue May 18, 2024 · 14 comments
Closed

cmd/go: not print not-defaults arch-env #67492

qiulaidongfeng opened this issue May 18, 2024 · 14 comments
Assignees
Labels
FixPending Issues that have a fix which has not yet been reviewed or submitted. GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@qiulaidongfeng
Copy link
Member

Go version

go version devel go1.23-38d8d01d3f Sat May 18 09:38:57 2024 +0800 windows/amd64

Output of go env in your module/workspace:

no need.

What did you do?

set GOAMD64=v3
./make.bat
..\bin\go env -changed

What did you see happen?

not print set GOAMD64=v3

What did you expect to see?

print set GOAMD64=v3

qiulaidongfeng added a commit to qiulaidongfeng/go that referenced this issue May 18, 2024
Fixes golang#67492
For golang#34208

Change-Id: I33a029f307d4a199dc338387cde41d49764f8fd7
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/586241 mentions this issue: cmd/go: print not-defaults arch-env

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. GoCommand cmd/go FixPending Issues that have a fix which has not yet been reviewed or submitted. labels May 18, 2024
@dmitshur dmitshur added this to the Go1.23 milestone May 18, 2024
@matloob
Copy link
Contributor

matloob commented May 20, 2024

I think the current behavior might be correct? When you run make.bat with GOAMD64=v3 you're essentially setting the default value for your go build to be v3. Then when you run env -changed, the default value is the same as the value in the environment so there should be no diff printed.

@qiulaidongfeng
Copy link
Member Author

But I use

set GOAMD64=v1
make.bat
set GOAMD64=v3
..\bin\go env -changed

still won't print set GOAMD64=v3

@qiulaidongfeng
Copy link
Member Author

For now, all arch-env , call EnvOrAndChanged the second parameter set now appears to be wrong, and its value is the relevant environment variable. (This means that there will be no non-default values because go will not get different values in internal/buildcfg and cmd/go/internal/cfg for the same environment variable such as GOAMD64.)

@qiulaidongfeng
Copy link
Member Author

See https://go.dev/wiki/MinimumRequirements#amd64 , GOAMD64 default value is v1, set GOAMD64=v3 happen default value is v3 Nonconformity documentation.

@matloob
Copy link
Contributor

matloob commented May 21, 2024

Ultimately I think someone more familiar with dist and the GO microarchitecture variables should make the decision. But from my perspective the the "default" behavior is what happens when you don't change the environment.

Also we shouldn't be changing anything outside of the go command.

The behavior in

set GOAMD64=v1
make.bat
set GOAMD64=v3
..\bin\go env -changed

not printing set GOAMD64=v3 does seem incorrect. but we should be able to detect that without changing dist or the runtime

qiulaidongfeng added a commit to qiulaidongfeng/go that referenced this issue May 21, 2024
Default values for GOARM were not fixed.
Since its default value is obtained in cmd/dist.xgetgoarm,
Dependent variable Settings are too complicated,
Copying is difficult to guarantee semantic equivalence.

For golang#67492

Change-Id: Ib8c7ed52fcc4bfae39577cb851e306180a6ab1c3
@qiulaidongfeng
Copy link
Member Author

not printing set GOAMD64=v3 does seem incorrect. but we should be able to detect that without changing dist or the runtime

Most arch-env can, PR fix has been sent.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/587177 mentions this issue: cmd/go: print some not-defaults arch-env

@qiulaidongfeng
Copy link
Member Author

@qiulaidongfeng qiulaidongfeng changed the title cmd/go: not print not-defaults arch-env when set not-defaults arch-env bootstrap or not set not-defaults arch-env bootstrap cmd/go: not print not-defaults arch-env Jul 2, 2024
@qiulaidongfeng
Copy link
Member Author

qiulaidongfeng commented Jul 2, 2024

I now confirm that when GOAMD64=v3, the default value of GOAMD64 is v3. If CL 586241 is going in the right direction, I can modify CL to ensure that the go command prints the default correctly and retain the current behavior of bootstrapping-when GOAMD64=v3, which defaults to v3.

qiulaidongfeng added a commit to qiulaidongfeng/go that referenced this issue Jul 3, 2024
Fixes golang#67492
For golang#34208

Change-Id: I33a029f307d4a199dc338387cde41d49764f8fd7
@matloob
Copy link
Contributor

matloob commented Jul 3, 2024

I'm uncomfortable with exporting the defaultGO variables in buildcfg, but I can't think of a better way to do this. I wonder what the compiler team thinks about this. @cherrymui what do you think about exporting the defaultGO variables in buildcfg. Do you think that's okay?

@cherrymui
Copy link
Member

I'm okay with exporting the defaultGO variables. It is an internal package anyway, and it is fine to be used by cmd/go. But I'm not really sure if we want to comparing them as strings. E.g. On ARM, the default is GOARM=7,hardfloat, but if you set GOARM=7 you get the same. Do we say it is "changed"?

@qiulaidongfeng
Copy link
Member Author

But I'm not really sure if we want to comparing them as strings. E.g. On ARM, the default is GOARM=7,hardfloat, but if you set GOARM=7 you get the same. Do we say it is "changed"?

The current implementation running "go env-changed" will print because 7,hardfloat, and 7 are really two different values. (Whether the semantics are the same is not the same as whether the values are the same), CL will not be after, because exporting the defaultGO variables contain DefaultGOARM=7.

@matloob
Copy link
Contributor

matloob commented Jul 12, 2024

@cherrymui @qiulaidongfeng I think for the purposes of go env -changed printing that the value has changed if the strings have changed but the value hasn't is harmless. Similar to what @qiulaidongfeng said, we print that the value has changed for other variables when the values are different and the semantics are the same so I think it would be reasonable to do the same here? I think it would be reasonable to expect that anyone using go env -changed from a command line or script would understand that the semantics might not be different even if the values are.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FixPending Issues that have a fix which has not yet been reviewed or submitted. GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants