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

spec: packages "unsafe" and "C" cannot be vendored #13703

Closed
alandonovan opened this issue Dec 21, 2015 · 13 comments
Closed

spec: packages "unsafe" and "C" cannot be vendored #13703

alandonovan opened this issue Dec 21, 2015 · 13 comments
Milestone

Comments

@alandonovan
Copy link
Contributor

Can we document in the specification of the Go 1.5 vendoring feature that the packages "unsafe" and "C" are special and cannot be vendored? The tools all currently make that implicit assumption.

@rsc rsc added this to the Go1.6 milestone Dec 22, 2015
@rsc rsc self-assigned this Dec 22, 2015
@rsc
Copy link
Contributor

rsc commented Dec 22, 2015

FWIW this would probably go into the go command docs or the vendor design doc, not the spec.

@alandonovan
Copy link
Contributor Author

Yes; I was thinking the vendor design doc.

@robpike
Copy link
Contributor

robpike commented Dec 22, 2015

The spec already says that unsafe is known to the compiler, which I think covers it. But I agree that the go tool's documentation for vendoring should call out unsafe and C. I also am unsure what happens if you vendor any dependencies of the compiler's generated code, like runtime, reflect, syscall, ...

@ianlancetaylor
Copy link
Contributor

@alandonovan Vendoring is now documented in cmd/go/help.go.

@rsc
Copy link
Contributor

rsc commented Dec 22, 2015

I am really not sure that unsafe should be special. Yes, "unsafe" is known to the compiler, but the compiler just follows rules given to it by the go command. Strictly speaking the vendoring spec suggests that in a program where there is a controlling "vendor/unsafe" directory, importing "unsafe" should get that package, the same as "vendor/math" and "math". If the go command tells the compiler that unsafe maps to vendor/unsafe, why shouldn't the compiler accept this? It would be a simple matter of swapping a few lines in the lookup. I cannot say that I placed the mapping lookup after the check for "unsafe" for any particular reason. In fact, if you create $GOROOT/src/vendor/unsafe/unsafe.go containing 'package unsafe' and then run 'go list -json reflect', it says that the import and dependency refer to 'vendor/unsafe'. But then when you compile the code, the go command instructs the compiler to interpret unsafe as vendor/unsafe, but the compiler ignores this direction. So the tools (the go command and the compiler) disagree.

I also don't understand why supporting "unsafe" in other tools is particularly difficult. The tools already do some kind of check to find out whether "math" really means "vendor/math". They can do the same for "unsafe", no?

I'm similarly on the fence about "C". It's easy to make it work if you really want a "vendor/C" directory, but it's also reasonable to say that "C" is interpreted by preprocessing before the usual build.

I'm not saying we should necessarily allow vendoring of unsafe and C or not. I'm trying to understand the rationale for carving out an explicit exception here. It's not something I considered before, and the current behavior is accidental more than anything.

@griesemer
Copy link
Contributor

I expressed my doubt to Alan (in personal communication) that packages unsafe and C were to be considered like any other packages for the purpose of vendoring; and that the fact that they were not mentioned in the vendoring doc was merely accidental.

I agree that there's no difficulty implementation-wise.

However, package unsafe (and in some sense, package C) makes a package that imports it inherently unsafe - so there's strong implicit semantics here. Of course, a compiler could know that a vendored unsafe may be different, or perhaps the compiler looks for the unsafe built-in functions. But at the moment at least, a look at the import statement suffices.

@alandonovan
Copy link
Contributor Author

I don't have a strong stake either way, but I do think we should specify the behavior.

The fact that all the tools seem to lean towards treating these packages specially suggests it would be easier to make that the standard behavior. A number of places in the go command and other tools assume that an import "C" means cgo, and the functions that perform this check don't necessarily have access to the import-path-to-package-path resolving logic. So it is inherently simpler to make the C and unsafe pseudo-packages special (w.r.t. vendoring; they are already special in other ways) and doing so doesn't lose any particular utility.

@griesemer griesemer reopened this Dec 22, 2015
@griesemer
Copy link
Contributor

Apologies for accidentally closing. That button is just totally misplaced.

@rsc
Copy link
Contributor

rsc commented Dec 22, 2015

On Tue, Dec 22, 2015 at 4:48 PM, Robert Griesemer notifications@github.com
wrote:

For what it's worth: The unsafe package is part of the spec

Yes, but its import path is not. Nothing about import paths is in the spec.

Russ

@gopherbot
Copy link

CL https://golang.org/cl/18438 mentions this issue.

rsc added a commit that referenced this issue Jan 8, 2016
…unsafe

There are fewer special cases this way: the import map applies
to all import paths, not just the ones not spelled "unsafe".

This is also consistent with what the code in cmd/go and go/build expects.
They make no exception for "unsafe".

For #13703.

Change-Id: I622295261ca35a6c1e83e8508d363bddbddb6c0a
Reviewed-on: https://go-review.googlesource.com/18438
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
@rsc
Copy link
Contributor

rsc commented Jan 8, 2016

After the linked CL, the compiler now agrees with cmd/go and go/build that "unsafe" can be vendored, and I have updated golang.org/s/go15vendor to make clear that the pseudo-package "C" cannot be vendored (and emphasize that "unsafe" can).

@rsc rsc closed this as completed Jan 8, 2016
@griesemer
Copy link
Contributor

PS: Even w/o vendoring, an unsafe package can now be installed and imported.

@alandonovan
Copy link
Contributor Author

Thanks. Seems like a good compromise.

@golang golang locked and limited conversation to collaborators Jan 7, 2017
@rsc rsc removed their assignment Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants