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

RFC: Check-in dependencies using non-gx paths #4831

Closed
Stebalien opened this issue Mar 18, 2018 · 14 comments
Closed

RFC: Check-in dependencies using non-gx paths #4831

Stebalien opened this issue Mar 18, 2018 · 14 comments
Assignees
Labels
topic/build Topic build topic/gx Topic gx

Comments

@Stebalien
Copy link
Member

Stebalien commented Mar 18, 2018

Currently, we check-in dependencies using gx paths. As far as I know, we do this to prevent users from running go get github.com/ipfs/go-ipfs and then complaining when the resulting binary doesn't work.

Unfortunately, leaving files in this rewritten state makes rebasing, merging, and reviewing painful. A simple dependency update tends to change a bunch of unrelated files. Worse, we occasionally end up importing gx packages that don't appear in packages.json.

Proposal:

  1. Don't commit changes in the rewritten state.
  2. Add a poison dependency that only builds with gx. This prevents users from building without gx (unless they know to remove this poison file). We could also make the non-gx version build but fail at runtime with a very obvious error message.

Drawbacks:

  1. We'd need to rewrite before building. Luckily, this only takes about 3s (on my machine at least).
  2. We'd probably want to build in a temporary directory to avoid rewriting the files the user is working on. However, I'm pretty confident we can do this without too much trouble.

I only bothered assigning those who I believe may hold strong opinions on this issue. Obviously, this impacts all contributors.

@whyrusleeping
Copy link
Member

As far as I know, we do this to prevent users from running go get github.com/ipfs/go-ipfs and then complaining when the resulting binary doesn't work.

Correct

Add a poison dependency that only builds with gx.

That should work, i'm okay with this. We can probably figure out a nice way to provide a useful error message too.

@kevina
Copy link
Contributor

kevina commented Mar 20, 2018

I am okay with this but can see several possible complications. Here is a laundry list of things I thought of:

  1. I agree that we should build in a temporary directory however:

    1. I regular build with just 'go build' when actively editing files to catch obvious errors as the dependency fetching add time and more importantly unwanted output. By building in a temporary directory a simple go build will no longer work. Therefore would really appreciate a build target that builds with out fetching the dependencies, it doesn't have to be document in the output of make all but should be documented somewhere where a developer (such as me) can easily find it.
  2. How will we as developers build without the automatic rewriting so we can use unpublished versions of a dependency locally?

  3. How will the poison dependency interact with gx-go rewrite --undo?

@Kubuxu
Copy link
Member

Kubuxu commented Mar 20, 2018

regular build with just 'go build'

That is true, it will also probably mess with people's completion or IDEs. I love the idea but we need to be careful.

@magik6k
Copy link
Member

magik6k commented Mar 20, 2018

Another way to approach this would be to add pre/post commit hooks to do the (un)rewriting as needed.

@kevina
Copy link
Contributor

kevina commented Mar 20, 2018

I as a general rule do not like commit hooks that modify files in place. I have a commit hook that checks for "go fmt" but it doesn't do it, it instead refuses to commit and tells me how to correct the situation.

Now if there was a way to do the rewriting without touching the files than I am all for it.

@Stebalien
Copy link
Member Author

@magik6k unfortunately, that would cause trouble with, e.g., git status and git diff (and git add etc...).


@kevina

How will the poison dependency interact with gx-go rewrite --undo?

It's just a dependency where the version on github is broken but the version in gx works. From the perspective of GX, it'll look like a regular dependency.

How will we as developers build without the automatic rewriting so we can use unpublished versions of a dependency locally?

You'd be able to use gx-link as we do today. Alternatively, you could remove the poison file and build with go-build to use all local dependencies.


@Kubuxu, @kevina

That is true, it will also probably mess with people's completion or IDEs. I love the idea but we need to be careful.

Very good point.

I wonder if we could create a vendor directory out of gxed packages for development?

@djdv
Copy link
Contributor

djdv commented Mar 27, 2018

In terms of development workflow I have no major opinion.
But in terms of source management, I think this is important

leaving files in this rewritten state makes rebasing, merging, and reviewing painful

The whole point of a change can easily get lost in the cascade of import diffs, and those changes pollute a file's blame history, which in turn makes it harder to track down when/why something changed.

@kevina

I as a general rule do not like commit hooks that modify files in place.

I tend to agree, I'd much rather fail than have something automatically modified. While the latter is convenient, it can't always be trusted to follow your intention.

@Kubuxu
Copy link
Member

Kubuxu commented Mar 27, 2018

Can we use gx with vendor dir support for this?

@Stebalien
Copy link
Member Author

@Kubuxu probably. Unfortunately, we can't just symlink to gx/ipfs/... as the paths there are rewritten. We'd probably have to either copy (shudder) or symlink to some global store (e.g., ~/.cache/gx/) until we get fuse working...

Note: we still want to rewrite on build as it embeds the hashes in the final binary (makes debugging easier).

@kevina
Copy link
Contributor

kevina commented Mar 29, 2018

Um, not 100% sure I am following but I don't think depending on fuse for building is such a good idea...

@Stebalien
Copy link
Member Author

Um, not 100% sure I am following but I don't think depending on fuse for building is such a good idea...

That is, when the user calls gx vendor --link (or whatever we end up calling it), gx-go first tries to link vendor/github.com/a/b to /ipfs/QmId/... and, if that fails, to ~/.cache/gx/ipfs/QmId/... (installing the package there). Of course, we shouldn't even try doing that until we have a reliable fuse daemon.

@AndrewMcSwain
Copy link

As far as I know, we do this to prevent users from running go get github.com/ipfs/go-ipfs and then complaining when the resulting binary doesn't work

If go get github.com/ipfs/go-ipfs is not the correct method for building from source, then why is that displayed as the only option in the Readme? What is the proper way to build from source, if not using the method stipulated in the readme?

@Stebalien
Copy link
Member Author

The README says to run go get -u -d github.com/ipfs/go-ipfs and then make install. The first command won't build anything (due to the -d flag). The second command (make install) will install the dependencies and build.

Stebalien added a commit that referenced this issue Sep 6, 2018
And add a dummy package that refuses to build unless we're using gx.

fixes #4831

License: MIT
Signed-off-by: Steven Allen <steven@stebalien.com>
@ghost ghost assigned Stebalien Sep 7, 2018
@ghost ghost added the status/in-progress In progress label Sep 7, 2018
Stebalien added a commit that referenced this issue Sep 7, 2018
And add a dummy package that refuses to build unless we're using gx.

fixes #4831

License: MIT
Signed-off-by: Steven Allen <steven@stebalien.com>
Stebalien added a commit that referenced this issue Oct 18, 2018
And add a dummy package that refuses to build unless we're using gx.

fixes #4831

License: MIT
Signed-off-by: Steven Allen <steven@stebalien.com>
@andrew
Copy link
Contributor

andrew commented Mar 11, 2019

Looks like this can be closed now that #6038 has been merged

@magik6k magik6k closed this as completed Mar 11, 2019
@ghost ghost removed the status/in-progress In progress label Mar 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic/build Topic build topic/gx Topic gx
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants