Skip to content
This repository has been archived by the owner on Mar 6, 2020. It is now read-only.

Compiler output should be relative to a fixed point #406

Open
3 tasks
davecheney opened this issue Sep 22, 2015 · 10 comments
Open
3 tasks

Compiler output should be relative to a fixed point #406

davecheney opened this issue Sep 22, 2015 · 10 comments

Comments

@davecheney
Copy link
Contributor

davecheney commented Sep 22, 2015

Having the compiler output a unique file path when compilation fails has been a long standing issue, see #57, #79, #99.

#57 changed to make paths relative to $PROJECT, but this since regressed :(

#395 #377 addressed the regression, by making the output relative to the current working directory.

This is a tracking issue to come up with a better plan than the piecemeal solutions so far.

Requirements:

  • must handle -R, ie don't assume that the current working directory is inside the project.
  • must handle temporary files created in context.Workdir(), which may not be in the same filesystem namespace as the $PROJECT, so may not be compatible with filepath.Rel, ie. C:\TMP and D:\SOURCE\PROJECT. This also includes the generated test packages, and cgo.
  • must be tested, preferably without doing a full end to end test of cmd/gb, ie. whatever functions generate the file paths passed to the compiler should be unit tested.

/cc @fatih, @inconshreveable, @tianon

@inconshreveable
Copy link
Contributor

I think you mean #377 not #395?

#377 is both unit tested and properly works with context.Workdir() on Windows. (It only modifies paths that were already relative. Any absolute paths are left unchanged.)

Unfortunately, #377's behavior with -R is suboptimal. Paths relative to $pwd are probably not the most helpful in that situation. Worse, if you use -R and specify a path on a different drive on Windows, gb will now break.

I would suggest one of two alternatives:

  1. Paths should always be relative to gb project root
  2. Paths should always be relative to $pwd or, if supplied, the argument to -R

Edit: I'm ambivalent between these options.

@davecheney
Copy link
Contributor Author

@inconshreveable correct. To be clear, I am not saying your work on #377 is incorrect. I am grateful for it. This issue is to track the remaining work.

I would suggest one of two alternatives:

  1. Paths should always be relative to gb project root
  2. Paths should always be relative to $pwd or, if supplied, the argument to -R

Sadly I think the solution is going to be more complicated. gb will have to handle both relative and absolute paths. We should be using relative paths internally whereever possible, and probably relative to the project root. But I fear there will be cases where absolute paths are required when dealing with generated code from tests or cgo.

More generally, I am concerned there is too much path munging going on inside build.go and gc.go, which is #99. I think paths should be generated once, and transformed a little as possible after that.

@inconshreveable
Copy link
Contributor

Yep, I understand! I just wanted to summarize where #377 leaves the current state of the world for everyone.

You make a good point about the cgo/temp files. Perhaps it makes more sense to say:

  • All paths located outside of the gb project should be absolute
  • All paths located within the gb project should be relative, either:
    1. Paths should always be relative to gb project root
    2. Paths should always be relative to $pwd or, if supplied, the argument to -R

Does that seem more reasonable? I agree that the less path munging the better, especially if they can be created with the proper form.

@davecheney
Copy link
Contributor Author

@inconshreveable yes, I like that, paths outside the project should be absolute. I think as per #57 paths should be relative to the project root. Specifically the $cwd should never be used for path computation within go (cmd/gb is the exception as it has to do a lot of skutt work to unpack package globs and things like that). I think this will resolve any problems with -R.

The decision of what is inside or outside a project should not be based on the $cwd of the user. It should be part of the logic that generates the paths, things like build.go:objfile and build.go:pkgfile, and friends. There is probably a lot to be done there.

@inconshreveable
Copy link
Contributor

+1 that all sounds good to me

@tianon
Copy link
Member

tianon commented Sep 22, 2015 via email

@davecheney
Copy link
Contributor Author

Excellent. I plan to work on this, but probably not til after Gotham Go. Feel free to jump in if you like.

To give some background about my suggestion for how this should be tested, I view testing the output of commands by looking at stdout/stderr with about the same level of disdain as I view using selenium for doing front end testing -- it works, but it has a low cost to benefit ratio.

I'd like to try to do this another way, generate all the paths we need via helper functions, then unit test those, assuming the results from those functions remains unmolested this should give stable results without the need to use end to end testing.

@dradtke
Copy link

dradtke commented Oct 6, 2015

Ah, I wasn't aware of this issue when I created my PR. When I have some time I'll look through the proposal here to see if I can help out.

@davecheney
Copy link
Contributor Author

@dradtke thanks. I'm sorry about your previous PR. The goal of #406 is to try to always involve the compiler from at fixed point in the directory tree, so that the output from the compiler naturally falls into the correct place relative to $PROJECT.

ssmccoy pushed a commit to ssmccoy/gb that referenced this issue Nov 3, 2016
In an attempt to solve issue constabulary#406, run the compiler instead of from
the package directory from the root of the project. Make all paths
passed to the compiler relative to the project root.

This appears to work, and the tests still pass; but it corrects the
output of compiler errors and they now appear with filenames relative
to the `GB_PROJECT_DIR`.
@ssmccoy
Copy link

ssmccoy commented Nov 3, 2016

I have submitted what I believe to be a very simple fix for this problem. I am using it in our project and it appears to solve the problem. (PR #652)

My project does not make comprehensive use of all gb features, so please let me know if there's anything this may have broken. But the tests still pass and this looks like it works.

> make bin/kamke
gb build
# kamke/calculator
src/kamke/calculator/calculator.go:113: undefined: BROKEN
FATAL: command "build" failed: exit status 2
Makefile:39: recipe for target 'bin/kamke' failed
make: *** [bin/kamke] Error 1

This makes vim's compiler integration much happier.

ssmccoy pushed a commit to ssmccoy/gb that referenced this issue Nov 4, 2016
In an attempt to solve issue constabulary#406, run the compiler instead of from
the package directory from the root of the project. Make all paths
passed to the compiler relative to the project root.

This appears to work, and the tests still pass; but it corrects the
output of compiler errors and they now appear with filenames relative
to the `GB_PROJECT_DIR`.
ssmccoy pushed a commit to ssmccoy/gb that referenced this issue Nov 22, 2016
In an attempt to solve issue constabulary#406, run the compiler instead of from
the package directory from the root of the project. Make all paths
passed to the compiler relative to the project root.

This appears to work, and the tests still pass; but it corrects the
output of compiler errors and they now appear with filenames relative
to the `GB_PROJECT_DIR`.
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

5 participants