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

design considerations for a new go-clang/clang repository #1

Closed
sbinet opened this issue Nov 4, 2015 · 30 comments
Closed

design considerations for a new go-clang/clang repository #1

sbinet opened this issue Nov 4, 2015 · 30 comments

Comments

@sbinet
Copy link
Member

sbinet commented Nov 4, 2015

Sparked by sbinet/go-clang#15, it might be interesting to refactor go-clang to ease the auto-generation of the API, using go-clang itself.
Obviously some bootstrapping is involve.

This issue is meant to document the design process.

The proposal would be the following:

  • freeze the sbinet/go-clang repository and migrate it under (say) github.com/go-clang/bootstrap
  • iteratively shave and peel unneeded features off go-clang/bootstrap so only the required bits for parsing the clang/CIndex API are included
  • create a (say) go-clang/clang/cmd/go-clang-gen-api command, using go-clang/bootstrap, to parse the clang/CIndex API and generate a cgo package exposing a nice and idiomatic go API for clang/CIndex. put the result as (say) go-clang/clang

a nice proof of concept has been put together at https://github.com/zimmski/go-clang-phoenix by @zimmski.

Benefits

  • reduce code maintenance and code churn between various clang versions
  • maintenance of multiple clang versions

Considerations

  • coupling between go-clang/bootstrap and go-clang-gen-api ?
    • should they live in the same repository?
    • should go-clang-gen-api really be under go-clang/clang ?
  • should go-clang-gen-api be able to handle all clang/CIndex versions via a big switch clangVersion?
  • or rather have a clang-x.y branch for each version?
  • regenerate the bindings via a //go:generate some-cmd plumbing ?
@zimmski
Copy link
Member

zimmski commented Nov 5, 2015

First of all, I would like to invite @marxriedl to join the group, since he helped out with part of the code.

@zimmski
Copy link
Member

zimmski commented Nov 5, 2015

Right now I can only answer a few issues with certainty:

  • go-clang/clang is a good repository and package path for the resulting generation. The import name will be "clang" by default and the path makes sense.
  • go-clang/clang should have branches for each LLVM/Clang minor version e.g. 3.4, 3.7 if it can be tested by the CI (https://github.com/travis-ci/apt-package-whitelist/blob/master/ubuntu-precise).
  • The branch names should be chosen that package versioning such as e.g. http://labix.org/gopkg.in works
  • The master branch always holds bindings for the latest LLVM/Clang bindings

EDIT: I will carry on with refactoring the go-clang-phoenix repository. Hopefully finishing the last API generation problems today.
Also, I would like to be added as "Owner" of the group to make it clear that I am a maintainer of these repositories. As with other open source projects where I am involved I will double check with you on changes and nothing gets in without a PR review first.

@sbinet
Copy link
Member Author

sbinet commented Nov 5, 2015

agreed.

as for gopkg.in, yes, it is useful to be compatible (as in: name correctly the branches) but do you also envision gopkg.in to be the canonical import path? (and enforce it via vanity imports?)

@sbinet
Copy link
Member Author

sbinet commented Nov 5, 2015

(PS: I made you (@zimmski) "owner" of go-clang. apologies for the oversight)

@zimmski
Copy link
Member

zimmski commented Nov 17, 2015

Thanks.

I had little time over the last two weeks but I just now finished a big part of this little project. Clang 3.7 is working, tested and I made a branch for it https://github.com/zimmski/go-clang-phoenix/tree/v3.7 I did the same for 3.6 but 3.5 and 3.3 are not working yet because some packages need to be whiteflagged. I already made some TravisCI issues to whiteflag them.

One problem with gopkg.in that just came up is that we cannot use "dotted" versioning e.g. 3.5 needs to be "v35" not "v3.5". The problem is that if there are two branches called v3.5 and v3.6 gopkg.in can only access 3.6 since the selector does only work on major versions. Hope that is fine with you?

Concerning the import path: If a developer needs an "older" version of this binding and not the newest the developer needs to use the gopkg.in path. I do not see any other way?

I do not think that the old go-clang repository is needed for bootstrapping since the current phoenix/bootstrap branch shows that it already works for that purpose (AFAIK).

The "go-clang-generate" code does not need to be included in every branch but I see no harm in including it unless we want *really clean branches (?). It could then just rest in the bootstrap branch so that the master and the version branches are simply just bindings. The current generation code handles some 3.6 and 3.7 quirks which do not interfere with the 3.3,3.4 and 3.5 generation. So right now we can handle all versions in one code base. So it would be currently possible that the version and the master branches just hold the bindings + headers. What do you think?

Remaining TODO:

  • Upgrade the master branch to 3.7
  • Document the development process
  • Document the release process for new clang versions (using the bootstrap branch which will stay at 3.4 for now)
  • Document the "update" process after go-clang-generate changes have been made
  • Review the phoenix repository @sbinet this has to be done by you
  • Rename the phoenix repository to go-clang/clang
  • Announcement (how should we handle this?)
  • Deprecate the old go-clang (?)

I think that covers everything, @sbinet what do you think? Anything to add?

@zimmski
Copy link
Member

zimmski commented Nov 18, 2015

@sbinet: Everything except the move to the go-clang group, the announcement and the handling of the handling of the old go-clang repository is done now :-) Looking forward to your feedback!

@sbinet
Copy link
Member Author

sbinet commented Nov 18, 2015

  • wrt gopkg.in/clang.vXY, I guess it is fine to drop the dot between the major and minor version numbers.
  • wrt the gopkg.in import path, my concern is the following: right now, gopkg.in 's usage hasn't percolated through the community. it is still maintained and used but it hasn't seen wide gains in mindshare. go-clang/clang is meant to be used as a library so people wanting an old version can easily vendor it (even more so with the vendor mechanism in go-1.5 and later.) So I'd err on advertising github.com/go-clang/clang as the canonical import path, advertising that gopkg.in/clang.v3X should work on a best effort basis (ie: provide both v3X and eg clang3.X tags) and recommanding that vendoring should be used (instead?) I mean: AFAICT, go-clang/clang won't be readily go get 'able, at least not in the general case (because of llvm-config, BTW: perhaps we should push upstream the generation of a pkg-config file?) so wrangling with gopkg.in is at this point just an uneeded additional dimension to the state/test-matrix... (not to mention additional user support headache if/when 2 versions of the same package are installed under different import paths)
  • wrt sbinet/go-clang as a bootstrap: I am fine with just dropping it completely (freezing and redirecting to go-clang/clang in the README.md)
  • wrt go-clang-generate: I believe it is best if a given code generator version lives with the code it generated, tied together via git. It will be easier for code regression detective work. So I am fine with having everything together, for all branches.

Now, for the review, I would propose to initialize an empty repository go-clang/clang and have you submit piece-wise PRs, first for the generator and then for each of the clang-v3X versions we want to support.
Or would this be too much (kind of duplicated) work?

  • wrt the announcement, when the time comes, we would draft it (via a PR or something) and we would submit it to golang-nuts, reddit/golang and golangbridge.

@zimmski
Copy link
Member

zimmski commented Nov 18, 2015

  • I now understand your concerns with gopkg.in and I was under the impression that it is an accepted solution. However, I have another suggestion which might work better. We have the "go-clang" path already. We can use that by making the repository names shorter and putting the version branches in their own repositories e.g. the bindings for 3.5 land in github.com/go-clang/v35 (package names must start with a letter and cannot contain a dot), the bootstrap branch lands in github.com/go-clang/bootstrap and the "latest" binding lands in github.com/go-clang/latest. go-clang-generate would then use the bindings of bootstrap which would ensure that it is always usable. This would involve a lot of cross-repository work but the import paths would be much cleaner and there would be no nasty branch-handling. We would also need no additional project to handle the versioning.
  • I am not familiar with pkg-config do you have an example for a Go project using this?
  • Review: I would like to just move and rename the repository since we have a lot of open issues in the tracker that can be done by newcomers.
  • Announcement: Sounds good

@sbinet
Copy link
Member Author

sbinet commented Nov 18, 2015

wrt pkg-config, here is a blog post explaining how it works: http://www.goinggo.net/2013/08/using-cgo-with-pkg-config-and-custom.html
and here is one of my projects using it: https://github.com/gonuts/ffi/blob/master/cgoflags.go

concerning the review, there are tools to migrate issues from one repository to another: https://github-issue-mover.appspot.com/ (source over there: https://github.com/google/github-issue-mover) (I tested it for one of my projects. it worked) or https://github.com/IQAndreas/github-issues-import
also, using either https://github.com/github/hub/tree/master/github or https://godoc.org/github.com/google/go-github/github doesn't seem too complicated to migrate issues from one repo to the other (in go, even.) Here is a first PoC: https://github.com/sbinet/gh-mv-issues

wrt the repo layout, I agree it is somewhat cleaner, naming-wise, but I fear this would seriously hinder detective work and code archeology.
importpath-wise, I believe it is still possible to play tricks with the go-import directive (documented here) and setup a go-clang.github.io/clang import-path (and go-clang.github.io/v3.7/clang for a given version) but still have the repo served under github.com/go-clang/clang.

@zimmski
Copy link
Member

zimmski commented Nov 18, 2015

I fear I have to be a little disappointing with this comment ;-)

pkg-config would be neat because it really makes cgo projects go-getable but I searched and I cannot find a .pc file for clang anywhere. Adding a .pc file to every clang installation, especially older ones, is not possible for us, so we cannot rely on it. Maybe we can ask for it in the next Clang version and then make its go-clang version go-getable?

Sorry, I do not think that it is worth migrating the issues. I would simple transfer the ownership of the phoenix project to the go-clang group. That is just filling out one form. Also the history and the blame-information is then still intact. So I would rather make the repository as good as possible in the current location, then transfer it and change all mentions of "go-clang-phoenix" to just "go-clang".

@zimmski
Copy link
Member

zimmski commented Nov 18, 2015

The suggested repo layout would make the detective work easier since the user can just say "I am using version xyz of go-clang". I do not see any problems besides that. We are a heavy user of go-clang and besides lacking support for newer Clang versions we did not have any problems that were on the side of go-clang.

The import path meta tag does not allow the mentioning of branch names. At least I did not see it mentioned in the documentation. So we would need to have different repositories/folders anyway.

If you are concerned about the import name being "v37" or "latest" we could just state in the README that users should use an explicit import name of e.g. clang. A subfolder clang would be also possible on our side but I hate stuttering in the import path.

@sbinet
Copy link
Member Author

sbinet commented Nov 18, 2015

yes, pkg-config support would only be introduced for clang-3.X where X>7 (and presumably have this be handled at the go-clang-generate level)

wrt phoenix -> go-clang ok, fine.
I'll send issues/PRs against phoenix during my review and then when everybody is fine with its state, we'll transfer it over to go-clang.

wrt repo layout. do we agree on something along the lines of:

  • github.com/go-clang/vX.Y bindings for each clang-vX.Y version (the package itself could live directly under /vX.Y or under /vX.Y/clang. in any case, it should be called clang)
  • github.com/go-clang/cmd repo where go-clang-generate would live (I would make the bootstrap bindings an internal package of the go-clang-generate command to have more leeway wrt possible API changes)
  • github.com/go-clang/clang (or /latest) repo where the bindings for clang-master would live

(you're right about tags and branches for go-import)

one thing though: when userA uses v3.5 and finds a bug in our bindings, where should she file the issue?
against the go-clang/v3.5 tracker? the /cmd one? or the /clang one? presumably against the /cmd one, right?

ok. I'll stop my bikeshedding here and start diving into phoenix code :)

@zimmski
Copy link
Member

zimmski commented Nov 18, 2015

Just a warning. The phoenix project still needs a lot of refactoring (especially headerfile.go) and there are still some clang specific lines in the generic generate code. However this is all internally and does not concern the generated binding. Also, the issues in the tracker describe some missing user features.

@sbinet
Copy link
Member Author

sbinet commented Nov 18, 2015

ack :)

@zimmski
Copy link
Member

zimmski commented Nov 19, 2015

I add an issue for pkg-config. It would be also OK for me to add some logic to go get so that we can set the CGO flags using a script but I do not know of any hook. Maybe we should ask about this on the mailing list?

repo-layout:

  • Every path item for a go path needs to be a package name according to the Go specification so we cannot use dots. Unfortunately it must be /vXY or /vX_Y
  • So you always want /clang as the last path item? We can simply do that by adding a folder to the version repositories.
  • I would like to see the generate code under github.com/go-clang/generate and then the command under github.com/go-clang/generate/cmd/go-clang-generate. This would obey a common Go convention which also leads to a nice binary name, and would allow to use the generate package in other applications.
  • I do not like stuttering in package names so I would prefer /latest

the generate command
I had a quick chat with @marxriedl yesterday and he made me rethink if we should add the generate command to the version repositories. And maybe it is not a problem at all to simply not include it. The thing is, that without all the generate code the repositories are simply like the current go-clang repository and investigating errors and problems did work there too. Hence, our new version repositories could work without this code too. Does that make sense? I know that I am changing my mind here, but I did not considered this argument.

Issue tracking

  • I think the tracking should happen in the repository where the generate command lands since there will be the most changes. We can simply add this to the README.

@sbinet
Copy link
Member Author

sbinet commented Nov 19, 2015

  • could you point me to the part of the Go spec that has such wording? I mean, by that reasonning, every single import path rooted under github.com is invalid,
  • it's not that I want it and am hard bent on it, but I know it may confuse some tools (and perhaps some humans too) that github.com/go-clang/v3.5 points to a package named clang, whereas github.com/go-clang/v3.5/clang may be slightly more obvious,
  • I would cast my vote for the following layout:
$ cd github.com/go-clang
$ tree .
.
├── cmd.git
│   ├── go-clang-generate
│   └── internal
│       └── generate      // package generate, only import-able by siblings
├── latest.git
│   └── clang             // package clang, bindings for clang-master from upstream
├── v3.5.git
│   └── clang             // package clang, bindings for clang-3.5 from upstream
├── v3.6.git
│   └── clang             // package clang, bindings for clang-3.6 from upstream
└── v3.7.git
    └── clang             // package clang, bindings for clang-3.7 from upstream

(the .git are here just to denote actual git repositories)

unless you already have an idea of how many other applications would use the generate package, I would still err on having that API hidden and private to the go-clang-generate command.
In the end, the crown jewels of go-clang are the exported types and funcs from the vX.Y/clang packages: other applications (including applications under /go-clang) should use that, no?

(minor bikeshedding: I would call the generate package just gen. same for go-clang-generate: go-clang-gen. less possible interferences and mixups with the go generate command.)

wrt issue tracking: ok.

wrt CGo + go get hooks: I doubt this will ever gain traction on golang-nuts. That's what the pkg-config scaffolding is meant to tackle. (go get is meant to be something anybody can run without having to inspect the package before downloading+installing. if arbitrary hooks coming from arbitrary scripts can be inserted, then security goes out of the window. at least that's what I understood (and recall) from the discussion at that time.)

@zimmski
Copy link
Member

zimmski commented Nov 19, 2015

  • import path: You are right, just the last part of a path needs to be a package name since that will be a package! https://golang.org/ref/spec#ImportPath my apologies for that!
  • import path: The /clang folder is fine with me. This also eliminates the need to do a manual import name which was a concern.
  • gen: The rename to go-clang-gen sounds reasonable.
  • gen: I am using the gen code already internally (for C->Go conversions) and I would like to widen the support for other bindings. So I would prefer a gen repository over copying the code :-/
  • tree: Your tree leaves one issue open, from which repository do we "branch of" a Clang version? A bootstrap repository is still needed because of the manual written code of the binding and boilerplate like the README and a .travisci file.
  • cgo + go get: Maybe there is some hook in CGo we could use? We only need to run one Go function during go install to make the packages go-getable. However, I do not see something like that in the documentation? https://golang.org/cmd/cgo/

I am btw happy with discussing all this. This should be a solid rewrite. So I like to write down all concerns and tackle them one by one until we agree on solutions.

@marxriedl
Copy link

Time for me to take part in this conversation.

Regarding the tree proposed by @sbinet and @zimmski's issue with it:
there could be a blank clang version repository which contains the files mentioned (.travisci, README.md), i.e., the minimum of files that are needed to create a new clang-version.
For each new clang version we then branch from this repository.
Its naming could be along the lines of bootstrap-clang-version, blank-clang-version.

@sbinet
Copy link
Member Author

sbinet commented Nov 24, 2015

(only tangentially related: I plan to attend FOSDEM-2016 and also submit a go-clang abstract, unless you'd like to present it (you guys having done most -- if not all -- the work, I guess it'd be only fair :))

@zimmski
Copy link
Member

zimmski commented Nov 24, 2015

It is fine with me if you want to present the rewrite :-) I do not know yet if I can make it to this year's FOSDEM though.

Regarding the review: I have some additional time left for open source project for November (especially this Friday will be open). December on the other hand is pretty well booked because of private events. Are you reviewing the API bindings and the generation command separately? We could push the bindings first since they live in separated repositories than the generate command and they are not influenced by internal refactoring.

@sbinet
Copy link
Member Author

sbinet commented Nov 24, 2015

I was somewhat focusing on the generation command.
yes, it is fine to first push the bindings.

@zimmski
Copy link
Member

zimmski commented Nov 26, 2015

I want to do the following today:

  • -gen - The current phoenix repository is put there but all binding code is stripped away (clang-c/ testdata/ *gen.go and the manual written binding code)
  • *-bootstrap - The stripped parts are put in here + bindings for the 3.4 version
  • Write a README for *-bootstrap
  • Write some scripts to make the release process easier for our new scheme
  • *-v3.4, *-v3.6 and *-v3.7 with bindings (forked of *-bootstrap) for the designated Clang versions
  • Adapt the README of *-gen

I can do that in the go-clang group or in my namespace and then just move the repositories. Anyway, the version repositories should then just hold the bindings so we can move them right away or after a review.

@marxriedl
Copy link

ack.
I don't particularly care about the namespace, either way is fine

@zimmski
Copy link
Member

zimmski commented Nov 28, 2015

@sbinet: We finished the documentation and the release process:

If you agree with bootstrap and v3.7 we can move bootstrap and then I will generate the version repositories in the go-clang group. No need to move v3.7. And no need to move gen yet.

@sbinet
Copy link
Member Author

sbinet commented Dec 1, 2015

@go-clang/dev OK. let's do that.
better to have the PR on the go-clang side.

@zimmski
Copy link
Member

zimmski commented Dec 1, 2015

So we move gen too?

@sbinet
Copy link
Member Author

sbinet commented Dec 1, 2015

yes. (unless you disagree?)

@zimmski
Copy link
Member

zimmski commented Dec 1, 2015

Fine with me! I will move them ASAP and change the content and imports. Looking really forward to finalizing the rewrite :-)

@zimmski
Copy link
Member

zimmski commented Dec 1, 2015

The version repositories are now online and the CI is fine with them:

When go-clang/gen#73 is resolved I will also add versions for 3.3 and 3.5. I have it on my TODO to test it on an internal project which is a heavy user of go-clang. @sbinet do you know of any projects that are using go-clang so that we can test them?

I put the announcement in #5. So I think we can now close this issue?

@sbinet
Copy link
Member Author

sbinet commented Dec 1, 2015

the known (to godoc at least) importers of sbinet/go-clang are:
https://godoc.org/github.com/sbinet/go-clang?importers

ie:

I've created #6.

Let's close this.

@sbinet sbinet closed this as completed Dec 1, 2015
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

No branches or pull requests

3 participants