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: cmd/cgo: create temporary objdir by default #64621

Closed
monoidic opened this issue Dec 8, 2023 · 6 comments
Closed

proposal: cmd/cgo: create temporary objdir by default #64621

monoidic opened this issue Dec 8, 2023 · 6 comments
Labels
Milestone

Comments

@monoidic
Copy link

monoidic commented Dec 8, 2023

Proposal Details

As a part of an invocation of go tool cgo, temporary files are created and placed in an "object directory". This directory can be specified with the -objdir flag, and it defaults to _obj within the current directory.

There are some issues with this:

  1. It is racy in conditions where this tool is invoked in parallel within the same directory
  2. it creates unexpected clutter (the objdir is not cleaned up at all post-invocation)
  3. it fails when the current directory is not writable (with a somewhat misleading error, e.g "Fatal error: can't create _obj/cgo.o: No such file or directory"; the success of os.Mkdir is not verified)

I believe that creating a temporary directory and removing it afterwards would be a better default, as it would avoid these issues.
Anything that explicitly sets the -objdir flag would be left unchanged, and the current default behaviour could be restored simply by explicitly setting -objdir to _obj

This would break anything that runs go tool cgo without explicitly setting -objdir and assumes that the _obj directory is left over afterwards. In the golang/go repository, there is no occurrence of this, aside from one behaviour test for cmd/go

@gopherbot gopherbot added this to the Proposal milestone Dec 8, 2023
@monoidic
Copy link
Author

monoidic commented Dec 8, 2023

I also (prematurely) created PR #64591 for this.

@bcmills
Copy link
Contributor

bcmills commented Dec 8, 2023

go tool cgo is not really intended to be called directly by Go users: generally one is expected to use go build instead.

@monoidic
Copy link
Author

go tool cgo is not really intended to be called directly by Go users: generally one is expected to use go build instead.

I think that, regardless of this, it would be a better default. The fact that it's racy by default does not seem to be documented anywhere for people that aren't typical Go users, and there are various workarounds to the left-over _obj directories being left over in various places. Examples from this repo include the gitignore file and bug #2693.

If this proposal is not accepted, then two potentially less controversial fixes (which would be addressed by this) still pop to mind:

  • document the by-default racy nature of calling go tool cgo within same directory
  • exit out early when the _obj directory is not successfully created because os.Mkdir fails

@folays
Copy link

folays commented Dec 22, 2023

go tool cgo is not really intended to be called directly by Go users: generally one is expected to use go build instead.

I have file like this :

//go:build structs_gen

package mystructs

/*
#include "some_structs.h" // It only defines simplistic C structs
*/
import "C"

//go:generate sh -c "go tool cgo -godefs mystructs_gen.go > mystructs_def.go"
//go:generate rm _obj/_cgo_.o
//go:generate rmdir _obj

type myStructA C.struct_structA
type myStructB C.struct_structB
type myStructC C.struct_structC

I do that because after 6 years, my IDE (GoLand) still doesn't offers completion to C structs. (neither on C.struct_* nor on their type aliases)

So I workaround this sorry state by //go:build-ignoring the very .go file which would "import" the C structs declaration/definitions to the Go package,

And instead I call go generate -tags structs_gen whenever I need to refresh the Go "import", which generate a .go file which the IDE takes into account (which would roughly be the same than the one auto-generated by go run/build, but for which the IDE would not account).

Doing so leaves a (not temporary) _obj/ + _obj/cgo.o file, which I rm with further //go:generate rm .... commands.

So, anyway, that's an use case for possibly not having a (persistent by default) _obj/ directory placed in the source tree.

Best Regards,

@monoidic
Copy link
Author

monoidic commented Jan 12, 2024

I had a similar issue and a similar solution, though with a dedicated code parsing tool.
cgo code is seemingly simply just not parsed at all by golang.org/x/tools/go/packages (returning just errors), perhaps with #21712 as the root cause.

Though gopls under VS Code seems to understand cgo definitions just fine, and I never really thought to look into how it did it...

monoidic added a commit to monoidic/go that referenced this issue Jan 12, 2024
monoidic added a commit to monoidic/go that referenced this issue Jan 12, 2024
monoidic added a commit to monoidic/go that referenced this issue Jan 12, 2024
@gopherbot
Copy link

Change https://go.dev/cl/548076 mentions this issue: cmd/cgo: create temporary objdir by default

monoidic added a commit to monoidic/go that referenced this issue Jan 12, 2024
@monoidic monoidic closed this as completed Apr 7, 2024
@monoidic monoidic closed this as not planned Won't fix, can't repro, duplicate, stale Apr 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants