Skip to content
This repository has been archived by the owner on Nov 13, 2021. It is now read-only.

gofpdf: add support for Go modules #185

Merged
merged 2 commits into from
Jul 24, 2018

Conversation

sbinet
Copy link
Contributor

@sbinet sbinet commented Jul 24, 2018

No description provided.

@jung-kurt
Copy link
Owner

Thanks, @sbinet. Go modules are new to me -- where can I read up on them? Do they pose a problem for people using older versions of Go?

@sbinet
Copy link
Contributor Author

sbinet commented Jul 24, 2018

the initial overview is there:

Dave Cheney has a little tutorial there as well:

and I also noticed this slightly more complete intro:

finally, there's the official WIP documentation:

theoretically, adding this file shouldn't impact users (as long as we don't use "modules" semantics. we don't yet.)

@jung-kurt
Copy link
Owner

Thanks for the links. I think the new module scheme provides a welcome remedy for problems caused by the GOPATH environment variable.

One of my goals with gofpdf is to keep it dependent only on the standard library. The motivation for the contrib directory was to isolate any add-ons that had outside dependencies. Consequently, I think a problem with your go.mod is that it requires packages like github.com/boombuler/barcode at too high a directory level. This should be required only for builds that will actually use barcodes. If I understand modules correctly, go will look for go.mod in the current build directory, then the parent, then the grandparent, etc. I think it makes sense to put a tailor-made go.mod in the contrib subdirectories and require only the packages that are specifically needed.

Also, I don't recognize github.com/pmezard/go-difflib and github.com/stretchr/testify. Are they packages that you think will improve testing?

@sbinet
Copy link
Contributor Author

sbinet commented Jul 24, 2018

Also, I don't recognize github.com/pmezard/go-difflib and github.com/stretchr/testify

these packages are marked as // indirect.
they come from one of the dependencies of gofpdf (it's ruudk/golang-pdf417.)
as that dependency hasn't have (yet) support for Go modules, the go mod tool pulls them in with their current hash.
once ruudk/golang-pdf417 gets Go modules support, these // indirect dependencies should disappear.

I think it makes sense to put a tailor-made go.mod in the contrib subdirectories and require only the packages that are specifically needed.

gotcha. I'll try that.

@sbinet
Copy link
Contributor Author

sbinet commented Jul 24, 2018

ok. I just dropped the extra deps of gofpdf proper.
I also dropped the module for contrib as "sub-modules" support seems to be somewhat subpar.

@jung-kurt jung-kurt merged commit c4fb503 into jung-kurt:master Jul 24, 2018
@jung-kurt
Copy link
Owner

Thanks, @sbinet!

@brunetto
Copy link

brunetto commented Sep 5, 2018

Hi,
I think this is not working when one needs to import contrib packages.
For example I got

./../../../../go/pkg/mod/github.com/jung-kurt/gofpdf@v1.0.0/contrib/barcode/barcode.go:29:2: unknown import path "github.com/boombuler/barcode/ean": cannot find module providing package github.com/boombuler/barcode/ean

Moreover, that package has go.mod in the master branch but not in the latest release.

Any suggestion @jung-kurt , @sbinet?

@jung-kurt
Copy link
Owner

Is this a consequence of having the contrib subdirectory be a member of the main gofpdf directory? Would the problem be corrected if the contrib tree was moved to its own repository? If so, I would rather do this than make the main project be dependent on third party libraries.

@brunetto
Copy link

brunetto commented Sep 5, 2018

Yes, it is possible to move contrib to a separate repo or to move the jung-kurt/gofpdf package under a folder that is not a parent of the contrib folder.

@jung-kurt
Copy link
Owner

jung-kurt commented Sep 5, 2018

I fear changing the import path of the gofpdf package would break a lot of projects. I favor moving the contrib to its own repository. Can commit history for the contrib files be carried over if that is done?

@brunetto
Copy link

I'm sorry @jung-kurt , I forgot about the question.
May be this can help https://help.github.com/articles/splitting-a-subfolder-out-into-a-new-repository/.
Should I file a new issue to track this?

@jung-kurt
Copy link
Owner

Thanks. Yes, I think a new issue makes sense.

@twpayne
Copy link

twpayne commented Feb 21, 2019

Versioning allows us to make breaking changes. A major version bump indicates a breaking API change such as changing the import paths. Therefore, I think it's fine to change some import paths as long as it's done with a major version bump.

Therefore I recommend:

  • Tag the current version (call it A).
  • Split the contrib directory into a separate repository. You can use git filter-branch to create a new repo that preserves history from this one, or clone this repo there and then delete everything that isn't in contrib.
  • Add a go.mod for the core.
  • Tag a new major version (call it B).

Existing users who already use a module versioning system (go modules, dep, vendoring, Bazel, or whatever) will be unaffected by the change and can migrate from A to B at their leisure. Changing a few import paths is quite trivial and can be done in a line or two of sed.

@jung-kurt
Copy link
Owner

Many thanks for the well-reasoned road map and for the git instructions. This looks good to me and I will try to get to it this weekend.

@twpayne
Copy link

twpayne commented Feb 21, 2019

That would be fantastic, many thanks for your work :)

@sbinet sbinet deleted the go-modules branch February 21, 2019 16:58
@jung-kurt
Copy link
Owner

I have split off the contrib directory to gofpdfcontrib. Also, I created a v2 branch of gofpdf with the contrib directory removed and documentation adjusted accordingly. The v2 bump is needed because gofpdf was already tagged at 1.x.y, and this change with contributed packages breaks compatibility.

I think the remaining steps include tagging a v2 release and setting the v2 branch as the default. (Alternatively, the v2 branch could be merged into master.) My questions relate to how go interacts with git.

  • The go wiki entry for modules specifies the need to include 'v2' in the import path. How is this done? Simply editing the go.mod file?
  • Once v2 is set as the default branch, or is merged over to the current default (master), users of gofpdf that don't import any of the contrib packages should have no issues. Packages that do import one or more contributed packages will encounter an error. Is it sufficient to explain in the README what has happened with this change, or is there a more graceful way to handle this?

@jung-kurt
Copy link
Owner

As a followup: I have edited the go.mod file to include the /v2 suffix:

module github.com/jung-kurt/gofpdf/v2

This is part of the new v2 branch. The files in gofpdfcontrib have been updated to import github.com/jung-kurt/gofpdf/v2.

The default branch will continue to be master (not v2) so that $GOPATH users won't see any change. Bug fixes will be made there and in v2, but new functionality will go only into v2.

@sbinet
Copy link
Contributor Author

sbinet commented Feb 25, 2019

IIRC, latest patch versions of 1.9 and 1.10 have support for the "github.com/jung-kurt/gofpdf/v2" import paths.
so, theoretically, one could merge v2 into master.

@jung-kurt
Copy link
Owner

My thought in keeping v2 out of master for awhile is to let GOPATH users who import contrib/barcode from getting an error. Does github provide any metric on the number of users this is?

@sbinet
Copy link
Contributor Author

sbinet commented Feb 25, 2019

I don't think github does.
but godoc does, to some extent:

@jung-kurt
Copy link
Owner

I don't think github does.

Had I thought about that, I wouldn't have been able to tease part users of barcode from gofpdf anyway.

but godoc does, to some extent:

Not a ton of packages. Maybe it just makes sense to document the change and make the merge.

@sbinet
Copy link
Contributor Author

sbinet commented Feb 25, 2019

yep, but the packages godoc mentions are only the packages (that import barcode and) for which the documentation has been accessed through godoc.

@jung-kurt
Copy link
Owner

After v2 is merged into master, programs that use the barcode package will get the following compilation error:

cannot find package "github.com/jung-kurt/gofpdf/contrib/barcode" in any of...

This won't be too informative for those users, but if they refer back to the README they will see that the remedy is simple enough (changing .../gofpdf/contrib/... to .../gofpdfcontrib/...) . I don't like the idea of replacing the contrib packages with stubs that produce more descriptive runtime errors because that could potentially push the problem into production. Any ideas on how to make this change as graceful as possible?

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

Successfully merging this pull request may close these issues.

4 participants