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

Allow librespot-golang to be consumed as a new style Go module #10

Closed
wants to merge 1 commit into from

Conversation

benpye
Copy link
Contributor

@benpye benpye commented Nov 17, 2018

This pull request allows librespot-golang to be consumed as a new style Go module. I have only changed the import paths in librespotmobile, I don't think any other changes need to be made but it is possible that I am wrong. I don't have gomobile locally.

We have to add in the module substitutions here so that the modules can reference the working copy of each other, otherwise they would reference the git repo rather than the local copy. See https://github.com/go-modules-by-example/index/blob/master/009_submodules/README.md for an explanaition. Once it's upstream however the cross module references must reference the latest commit rather than the nonsense version I used, this does mean that if Spotify and librespot are updated together, there should be a commit updating just Spotify, followed by a commit updating librespot, I believe.

I think ultimately it would make more sense to restructure the code. Do we gain anything by having Spotify as a seperate module to librespot?

@ViRb3
Copy link
Contributor

ViRb3 commented Dec 22, 2018

It may be better to use the replace directive instead of hard-coding the GitHub URL in imports, it would make forking a lot easier. I will play around with it and propose the best solution.

@xplodwild
Copy link
Contributor

I suggest we hold onto this until Go 1.13 is released with official gomodules support. We can experiment with it however meanwhile, as I agree with @ViRb3 in that we shouldn't hardcode the github path in imports for easier forks (hence why I tend to go the "src" way, even if it's not necessarily optimal).

I'll play around with it, I haven't digged much into gomod yet

@anisse
Copy link
Contributor

anisse commented Feb 27, 2019

I don't think it's a good idea to wait. This issue (go.mod + go gettability) is today the biggest hurdle to try the library. Serving users of the library should come first, then serving forks.

Having the full canonical import part inside the headers might seem overkill, but anyone having local modifications can solve this with two or three "replace" directives in their local go.mod. Forks have to resort to what I did here: anisse/librespot-golang@377997a ; but that's only if they want go-gettability themselves.

Go 1.11 already has official gomodules support, and Go 1.12 was just released. Not much will change in Go 1.13, just the deprecation of the GOPATH.

@xplodwild
Copy link
Contributor

I'll give it a try soon and merge it if it works

@ViRb3
Copy link
Contributor

ViRb3 commented Feb 27, 2019

I have taken an alternative approach in my local fork that relies solely on the replace directive. It makes the repo fully location-independent, no hardcoded GitHub url. I will submit a PR for evaluation in a few hours.

@anisse
Copy link
Contributor

anisse commented Feb 27, 2019

@ViRb3 I'm curious to see how you did this!

@ViRb3
Copy link
Contributor

ViRb3 commented Feb 27, 2019

I'm afraid I just lost my laptop, I will be unable to do this for a while.

@anisse
Copy link
Contributor

anisse commented Feb 28, 2019

It looks like the policy to have fully-qualified import paths is here to stay, see golang/go#20883 (comment).

I'm afraid relying solely on the replace directive is only good for local development which is why I was curious to see if you had an alternate solution @ViRb3 .

I'd advise to use fully-qualified import paths like most go projects. Very complex libraries do that and it seems to work well for them.

@nezorflame
Copy link

nezorflame commented Mar 18, 2019

This is a good initiative, but in order to fully comply with it, this is not enough.
This repo needs a serious package structure rework (preferrably in the form of the Go project-layout) with proper import paths, and should start using versions, following SemVer 2.0 specification.
I was curious to give this lib a try in my pet project, so if you want, I could provide a PR implementing these changes (of course except versioning, which should be done by the maintainers). Current implementation needs to be marked as v0.x.y releases, and the final result of Go module migration could be marked as the first v1.0.0 version.

@diamondburned
Copy link
Contributor

I've made a PR on go.mod, issue @ #17

@xplodwild
Copy link
Contributor

This has been superseded by #17 and the associated PR

@xplodwild xplodwild closed this Apr 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants