[WIP] Descriptor refactor. by tbranyen · Pull Request #162 · nodegit/nodegit

tbranyen · May 7, 2014

This PR is working on completely decoupling the meta data required to generate the source binding from the JSON published by the libgit2 project. Once this is completed, we should be able to trivially add and remove new methods and classes as they change by simply updating the descriptor file.

So far it's going better than expected, libgit2 is so strict with the conventions that I can infer most manually added properties. This is a very important PR so time will be taken and it will not be rushed.

My goal is to improve the templates in a separate PR.

tbranyen · May 18, 2014

I'm currently stuck on figuring out the best way to access git_tree_entry objects. Inspecting how others handle this:

Both of these approaches are setting values directly on a hash/object and returning that. Instead, I'm considering the approach of native v8 accessors to dynamic properties as outlined here:

https://developers.google.com/v8/embed#dynamic

If this experiment ends up working for git_tree_entry's, I think it'll be worth considering overhauling how we handle objects like git_signature's that currently require function invocation for synchronous dynamic property lookups.

tbranyen · Jun 10, 2014

I've been experimenting with a new generated JSON descriptor file from the libgit2 project. @carlosmn has been putting work into https://github.com/libgit2/docurium using libclang's AST parser. I don't think it has all the information we need yet to fully generate git_tree_entry though.

3y3 · Jun 11, 2014

I try to build this branch, but received an error, about not found files diff_driver.c, diff_file.c... (11 items), all from libgit2. (I use npm install). I have an outdated libgit2 or binding.gyp is outdated?

tbranyen · Jun 11, 2014

Pretty sure I broke something with the latest commit. If you go back a commit or two it should build fine. Need to figure out a way to keep the binding.gyp in sync with the current version of libgit2. I think it should be pretty easy to get a list and dynamically inject into the .gyp file.

3y3 · Jun 12, 2014

This pr is very hard to review, how about divide it on two (firstly)?
I see here some useful commits, which we can merge now:

Better installation flow for developing.
Updated binding to match how git-utils handles libgit2.
Remove documentation on CMake.
Removed all generated C++.

tbranyen · Jun 12, 2014

Agreed, I can clean it up and make it much easier to review.

The most important task of this PR is to go from:

https://github.com/nodegit/nodegit/blob/descriptor-refactor/build/codegen/descriptor.json

to:

https://github.com/nodegit/nodegit/blob/descriptor-refactor/build/codegen/compare.json

The emphasis being on configuring as little as possible with the most accurate data for the templates to generate.

3y3 · Jun 20, 2014

generate/index.js

local not used anywhere

There's gonna be some weirdness as I'm rebasing this branch. Now that you're getting involved, I need to undo all my sloppy coding while I thought nobody was looking. I'll need a few more days to get it all in order for someone else to start reviewing and helping out.

Ok. I will continue in read only mode ;-)

tbranyen · Jun 23, 2014

@3y3 progress! I'm ready for you to start helping out with this PR. All tests I have uncommented are passing at the moment. We need to go through all remaining ones and tweak the generate/descriptor.json until they pass. We may need to tweak the templates too.

3y3 · Jun 23, 2014

@tbranyen , Ok, I did checkout. I think that best way to understand that happening here - fixing some tests - this is my strategy for now.

A somewhat experimental commit to demonstrate and lay the foundation for the new code generation structure. File breakdown: idefs.json (Generated) - Is a generated file that is substituting the heavily modified descriptor that nkallen had left us. nkallen.json (Will be deleted in the future) - Reference of the modified v0.18.0.json file @nkallen had created. descriptor.json (Developer maintained) - Inform decisions to the descriptor generation. setup.js - A script that runs to generate the idefs.json file. types.json (Will be deleted in the future) - Maps functions/classes/structs/types/etc. to JS, C, C++, and Emscripten (in the future).

That are also relevant to this branch.

I've been able to get most of the tests passing, just a handful left. There is a little manual intervention needed for most methods to get them to compile correctly. I'd love help on: - Getting more methods compiled and tested! - Code reviews. - Testing the code!

This allows node-gyp to clean the build directory without wiping out the code generation files.

tbranyen · Jun 30, 2014

I think I got Windows working. I had to reintroduce the additionalCast property I had put in the templates, but lost in a rebase.

For some reason the compiler is complaining it can't link git_pathspec_free when I include the pathspec sources. Not sure why, so I've commented out for now since it's unused.

3y3 · Jun 30, 2014

I confirm - nodegit was builded now. I try to inspect v0.11 compatibility, but it's also mistery.

tbranyen · Jul 1, 2014

@3y3 the issue appears to be caused by borrowing prototypes. For instance:

// Broken.
var oldBlob = Blob.prototype.lookup;
oldBlob(this, oid, function() { });

// Working.
Blob.lookup(this, oid, function() {});

I'm not entirely sure why this is.

tbranyen · Jul 2, 2014

Well that was a weird bug, but it's fixed now. Full 0.11 support again. Once I can figure out this segfault issue, I can merge this branch and we can start enabling more methods people want :)

3y3 · Jul 2, 2014

generate/templates/class.cc.ejs

replacement is not equivalent. Are you sure that null is acceptable?

Null is not acceptable, that's why this change exists:

> typeof null != 'undefined' true > null != undefined false >

3y3 · Jul 2, 2014

.gitignore

For windows:

/vendor/Release /vendor/*.vcxproj /vendor/*.filters /vendor/*.sln

3y3 · Jul 2, 2014

I finish review. See the last comments.
After this, I think, it will be ready to merge.

tbranyen · Jul 2, 2014

@3y3 still have to fix that one segfault in git_revwalk_push I don't know what's causing it still.

tbranyen · Jul 2, 2014

Figured it out, was a shouldAlloc where it shouldn't have been.

tbranyen · Jul 2, 2014

Okay cool, feedback to be addressed:

DiffList test.
Rename repo to repository.
Update gitignore for Windows.

Blocking:

Method parity with master. (separate PR)

Miscellaneous tasks:

Remove nkallen.json, save elsewhere as reference.
Polish descriptor.json, instead of inferring perhaps we should be more explicit?
Either remove or correct types.json, should not write to it in setup.js.
Cleanup and refactor setup.js.

- Updated gitignore for Windows. - Renamed repo => repository. - Fixed DiffList => Diff conversion and tests.

tbranyen · Jul 3, 2014

Okay cool, since we're at full testing parity, I'm going to merge this. We'll break up the remaining tasks to lead up to a new release. Since there's such a massive change under the hood, I'm thinking we'll bump minor to 0.2.0.

[WIP] Descriptor refactor.

3y3 · Jul 3, 2014

Congratulations! Great job! ;-)

tbranyen · Jul 3, 2014

@3y3 thanks, but still so much work to do! So glad to get this major blocker out of the way. Still need to document the process of adding new methods and what kinds of tactics can be used to fix unexpected bugs.

tbranyen · Jul 3, 2014

Also I'm pretty sure I messed up the Diff tests, need to revisit and figure out why I had to change numbers around to make things pass.

This was referenced May 7, 2014

Enable promises. #153

Merged

Emscripten binding templates. #142

Closed

Logo for project #69

Closed

tbranyen added the Feature Request label May 8, 2014

tbranyen changed the title ~~WIP Tracking the descriptor refactor work.~~ WIP Descriptor refactor. May 16, 2014

tbranyen changed the title ~~WIP Descriptor refactor.~~ [WIP] Descriptor refactor. Jun 9, 2014

tbranyen mentioned this pull request Jun 9, 2014

How to create a branch #174

Closed

3y3 mentioned this pull request Jun 10, 2014

NodeJS v0.11.13 compatibility #175

Merged

3y3 reviewed Jun 20, 2014
View reviewed changes

tbranyen closed this Jun 23, 2014

tbranyen reopened this Jun 23, 2014

tbranyen added 11 commits June 24, 2014 22:31

Automate the building of all libgit2 types.

96f0779

Library changes.

bf604c5

Enable GitAttr.

2082d7d

Automate dependencies.

699c34e

Generating more types and safe identifiers.

712a4dc

Copied changes from Emscripten branch.

6a4225b

That are also relevant to this branch.

Added more types.

d0b414d

Save state.

f59963d

Moved build/codegen to generate.

a2192c1

This allows node-gyp to clean the build directory without wiping out the code generation files.

Fixes for Windows.

aa43f3f

Convert prototype methods to constructor to fix 0.11

1730440

3y3 reviewed Jul 2, 2014
View reviewed changes

tbranyen added 2 commits July 2, 2014 09:53

Fixes for PR comments and some revwalk

bdc8e97

Fix lint errors and lint runner

f8f7305

3y3 reviewed Jul 2, 2014
View reviewed changes

.gitignore

Copy link

Contributor

3y3 Jul 2, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For windows:

/vendor/Release /vendor/*.vcxproj /vendor/*.filters /vendor/*.sln

Made revwalk push synchronous, fix segfault

cb83ac2

Fixes remaining blocking issues

6caa3e8

- Updated gitignore for Windows. - Renamed repo => repository. - Fixed DiffList => Diff conversion and tests.

tbranyen added a commit that referenced this pull request Jul 3, 2014

Merge pull request #162 from nodegit/descriptor-refactor

1bcb66c

[WIP] Descriptor refactor.

tbranyen merged commit 1bcb66c into master Jul 3, 2014

tbranyen deleted the descriptor-refactor branch July 3, 2014 14:54

Search code, repositories, users, issues, pull requests...

Conversation

tbranyen commented May 7, 2014

Uh oh!

tbranyen commented May 18, 2014

Uh oh!

tbranyen commented Jun 10, 2014

Uh oh!

3y3 commented Jun 11, 2014

Uh oh!

tbranyen commented Jun 11, 2014

Uh oh!

3y3 commented Jun 12, 2014

Uh oh!

tbranyen commented Jun 12, 2014

Uh oh!

3y3 Jun 20, 2014

Choose a reason for hiding this comment

Uh oh!

tbranyen Jun 20, 2014

Choose a reason for hiding this comment

Uh oh!

3y3 Jun 20, 2014

Choose a reason for hiding this comment

Uh oh!

tbranyen commented Jun 23, 2014

Uh oh!

3y3 commented Jun 23, 2014

Uh oh!

tbranyen commented Jun 30, 2014

Uh oh!

3y3 commented Jun 30, 2014

Uh oh!

tbranyen commented Jul 1, 2014

Uh oh!

tbranyen commented Jul 2, 2014

Uh oh!

3y3 Jul 2, 2014

Choose a reason for hiding this comment

Uh oh!

tbranyen Jul 2, 2014

Choose a reason for hiding this comment

Uh oh!

3y3 Jul 2, 2014

Choose a reason for hiding this comment

Uh oh!

3y3 commented Jul 2, 2014

Uh oh!

tbranyen commented Jul 2, 2014

Uh oh!

tbranyen commented Jul 2, 2014

Uh oh!

tbranyen commented Jul 2, 2014

Uh oh!

tbranyen commented Jul 3, 2014

Uh oh!

3y3 commented Jul 3, 2014

Uh oh!

tbranyen commented Jul 3, 2014

Uh oh!

tbranyen commented Jul 3, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants