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

proposal: go:embed interpolate go env #48348

Closed
codefromthecrypt opened this issue Sep 13, 2021 · 9 comments
Closed

proposal: go:embed interpolate go env #48348

codefromthecrypt opened this issue Sep 13, 2021 · 9 comments

Comments

@codefromthecrypt
Copy link

Right now, you can embed files with wildcards like so:

//go:embed proc_*.go
var procSrcFS embed.FS

This is a bit more loose than necessary, as I'd like to be able to enlist go env variables

//go:embed proc_{{goos}}.go
var procSrc string

Something like this would significantly reduce the code needed to embed something platform-specific, as we would expect a go env variable to have only one value.

@gopherbot gopherbot added this to the Proposal milestone Sep 13, 2021
@mvdan
Copy link
Member

mvdan commented Sep 13, 2021

You can accomplish the same with build tags:

$ cat embed_proc_windows.go
package p

//go:embed proc_windows.go
var procSrc string

@mvdan mvdan closed this as completed Sep 13, 2021
@mvdan mvdan reopened this Sep 13, 2021
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Sep 13, 2021
@codefromthecrypt
Copy link
Author

codefromthecrypt commented Sep 13, 2021

Making one go file per value indeed can work, and the IDE can resolve that just like it does now. It less code than a wildcard + scanning the files, but more files to maintain and drifty ones. Ex you need one of these per dimension, but they won't build on the current goos either.

Build flagged source has a main issue even in smart IDEs which is drift is super easy. Ex someone renames procSrc and nothing catches it as that part isn't activated, so they find out in CI. While it is true lack of a foo{{goos}} expression would also not be found except in CI, the values of the template are fixed and the variable if renamed improperly will break all builds, not just the current one.

Further, in the case where you have multiple expressions into the same embed.FS you would have to copy/paste all the expressions to each build flagged file to get the same contents of the variable. You would also have to make some comments to say "look over here also" to know where the rest of the variables are defined. Centralization of variables is its own goal and forcing people navigation through flagged files interferes with that. That often manifests in comments in ideal case or newbie confusion in less ideal.

Finally, while I didn't mention something besides {{goos}}, you could imagine others are useful too, {{goexe}} and while you can also do that with more build files it seems like it would get nasty fast.

@fzipp
Copy link
Contributor

fzipp commented Sep 13, 2021

This proposal introduces another way of conditional compilation. It's a compile-time switch that only applies to parts of a source file, like an #ifdef, which Go has avoided. To quote from The Practice of Programming (Kernighan & Pike), chapter 8.4 "Isolation":

[...] it is a mistake to have non-portable code scattered throughout a program: that is one of the problems that conditional compilation creates.
Localize system dependencies in separate files. When different code is needed for different systems, the differences should be localized in separate files, one file for each system.

@codefromthecrypt
Copy link
Author

It is an interesting way to think of this as "conditional compilation" and if we focused on the mechanics underneath the "go:embed" feature, that's probably an accurate way to find something to anchor a decision on.

I would challenge us to think of the user experience and be specific. For example, go:embed does not allow arbitrary compilation of code. For example, if we accepted a template expression here besides '*' which is already supported...

it wouldn't have the same risk or impact as a random if statement or adding a new variable. It would change the contents of that byte array, string or embed.FS, and that's quite a narrow scope compared to something like an ifdef, right?

Personally I think spreading out "go:embed" selection across multiple build flagged files is a cure worse than the disease of using "*". I wouldn't recommend it, even if I might with actual code inclusions (like platform-specific functions or variables)

IOTW, I don't really buy that we should think about go:embed expressions the same way as we think about including code statements. It is an interesting perspective, but I think that's more bottom up thinking vs UX centered, and I tend to bias towards the latter.

@mvdan
Copy link
Member

mvdan commented Sep 14, 2021

Your points about UX and duplication seem valid, but the bigger question as far as I can tell is whether this kind of use case will be very common. I at least haven't encountered it to this degree. In a couple of cases cases I have needed to embed different assets per OS, such as an ICO icon for Windows and a PNG icon for Linux/Mac, where using two files seems entirely reasonable.

The other side of the coin is what syntax you define, and whether it should apply to other //go:* directives. I don't think {{goos}} is a syntax we've used anywhere in the Go toolchain before.

@rsc
Copy link
Contributor

rsc commented Sep 15, 2021

Go programs should not be in the business of changing behavior based on arbitrary environment variables. Among other things, that would invalidate the build cache in unexpected ways.

@rsc rsc moved this from Incoming to Active in Proposals (old) Oct 13, 2021
@rsc
Copy link
Contributor

rsc commented Oct 13, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Decline in Proposals (old) Oct 20, 2021
@rsc
Copy link
Contributor

rsc commented Oct 20, 2021

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Oct 27, 2021
@rsc
Copy link
Contributor

rsc commented Oct 27, 2021

No change in consensus, so declined.
— rsc for the proposal review group

@rsc rsc closed this as completed Oct 27, 2021
@golang golang locked and limited conversation to collaborators Oct 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

5 participants