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

Inject XCADDY_GO_BUILD_FLAGS through go operations (#102) #110

Merged
merged 2 commits into from
Aug 16, 2022

Conversation

aabelix
Copy link
Contributor

@aabelix aabelix commented Aug 13, 2022

Commit 47f9ded is not sufficient alone to
ensure that all go modules are installed with write bit set because 'go mod'
or 'go get' might install submodules of other modules as read-only.

If set, the environment variable appends '-modcacherw' to go mod and go get
to ensure that submodules of other modules have write access.

@aabelix
Copy link
Contributor Author

aabelix commented Aug 13, 2022

Relates to #102 which was not fully resolved, yet.

@mohammed90
Copy link
Member

It's best if we don't make super specific flags for the various possible options. According to the go command docs -- emphasis mine:

The build flags are shared by the build, clean, get, install, list, run, and test commands:

The env var introduced earlier is concerned with build flags. What do you think of injecting the provided XCADDY_GO_BUILD_FLAGS across the various commands instead of injecting the -modcacherw as a separate flag? Does this resolve your issue?

@aabelix
Copy link
Contributor Author

aabelix commented Aug 14, 2022

Yes, that will also work. I will amend the change.

@aabelix aabelix force-pushed the bugfix/env-mod-cache-rw branch from bc80f02 to deb12e9 Compare August 15, 2022 09:02
@aabelix
Copy link
Contributor Author

aabelix commented Aug 15, 2022

@mohammed90, I think this patch version is better but it does not support build flags to all those commands that you suggested so I think I can improve this a bit. The only common interface that I could find was this

environment.go:

func (env environment) newCommand(command string, args ...string) *exec.Cmd {
	cmd := exec.Command(command, args...)
	cmd.Dir = env.tempFolder
	cmd.Stdout = os.Stdout
	cmd.Stderr = os.Stderr
	return cmd
}

so I could change this interface to append the build flags but I was not sure if you would be OK with it. The name of that method is very generic and it could be potentially used for running other than build-related commands. What do you think?

Commit 47f9ded is not sufficient alone to
ensure that all go modules are installed with write bit set because 'go mod'
or 'go get' might install modules as read-only, too.

If set, the environment variable is appended for the other go commands that
support build flags.
@aabelix
Copy link
Contributor Author

aabelix commented Aug 15, 2022

I guess this commit would be sufficient as a final solution?

@mohammed90
Copy link
Member

@mohammed90, I think this patch version is better but it does not support build flags to all those commands that you suggested so I think I can improve this a bit. The only common interface that I could find was this

environment.go:

func (env environment) newCommand(command string, args ...string) *exec.Cmd {
	cmd := exec.Command(command, args...)
	cmd.Dir = env.tempFolder
	cmd.Stdout = os.Stdout
	cmd.Stderr = os.Stderr
	return cmd
}

so I could change this interface to append the build flags but I was not sure if you would be OK with it. The name of that method is very generic and it could be potentially used for running other than build-related commands. What do you think?

A method like this can guarantee it's only using go command.

func (env environment) newGoCommand(args ...string) *exec.Cmd {
	return env.newCommand(utils.GetGo(), args...)
}

The method newCommand runs only go commands so let's make command 'go'
implicit and rename it to make it more verbose.
@aabelix aabelix force-pushed the bugfix/env-mod-cache-rw branch from deb12e9 to c7f9280 Compare August 16, 2022 07:15
@mohammed90 mohammed90 changed the title Add XCADDY_GO_MOD_CACHE_RW env var (#102) Inject XCADDY_GO_BUILD_FLAGS through go operations (#102) Aug 16, 2022
Copy link
Member

@mohammed90 mohammed90 left a comment

Choose a reason for hiding this comment

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

Looks good! I'll merge this as-is now, but I'll follow it up with another commit. mod supports modcacherw, modfile, and overlay of the build flags, BUT not the rest. The next commit will add XCADDY_GO_MOD_FLAGS which will only apply to go mod. In your case, you will need to add -modcacherw to both env vars.

Thanks for the work on the PR!

@mohammed90 mohammed90 merged commit edc1f41 into caddyserver:master Aug 16, 2022
@aabelix
Copy link
Contributor Author

aabelix commented Aug 16, 2022

Sounds good to me. Thank you for your help, Mohammed!

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