multitheftauto / mtasa-blue

Multi Theft Auto is a game engine that incorporates an extendable network play element into a proprietary commercial single-player game.
https://multitheftauto.com
GNU General Public License v3.0
1.41k stars 438 forks source link

Use submodules for dependencies #319

Open qaisjp opened 6 years ago

qaisjp commented 6 years ago

I've briefly discussed this on the development chat and I've created this issue so that we can properly evaluate whether we want to use submodules. Please share your thoughts.


Is your feature request related to a problem? Please describe. Updating and verifying vendor code is fairly manual work.

Describe the solution you'd like For "vendored" code we haven't modified we can use git submodules.

This would make it easier to update and verify dependencies as we no longer need to fiddle about with copying code and figuring out which files to keep. We would just need to update the submodule reference.

Some things to be aware of:

Additional context

qaisjp commented 5 years ago

This is a cut down version of the guide above and the Git book. The below information is intended to be added to a developer guide.

This content is not yet validated against mtasa-blue, it is content converted from output of a bare-ish test repository (test and test-2).


Important things to remember

Recommended pre-configuration

Other recommendations:

Working alongside submodules (changes to the current workflow)

Cloning mtasa-blue

Bringing your local repository up to date

Caveats if you do git pull:

Working with submodules directly

Adding a dependency

Updating a dependency

This section only covers updating mtasa-blue to a new commit of a remote dependency. It does not cover updating a forked dependency.

We follow the below instructions:

Therefore, I recommend splitting the process manually: first git fetch to get all new data from the remote in local cache, then log to verify what you have and checkout on the desired SHA1.

Removing a dependency

This rarely happens, and it's complicated!

Refer to the relevant section "Permanently removing a submodule" in "Mastering Git submodules".

What about submodule merge conflicts?

From "Merging Submodule Changes", 7.11 - Git Tools - Submodules:

If you change a submodule reference at the same time as someone else, you may run into some problems. That is, if the submodule histories have diverged and are committed to diverging branches in a superproject, it may take a bit of effort for you to fix.

If one of the commits is a direct ancestor of the other (a fast-forward merge), then Git will simply choose the latter for the merge, so that works fine.

Because submodules in mtasa-blue will (should) only contain consecutive commits on the master branch, you theoretically should never have to deal with this problem. Hopefully this problem never happens to you.

qaisjp commented 5 years ago

Dependency on build infrastructure

This issue requires us to upgrade the build system to use git instead of svn.

See output below (some lines converted to [...] for brevity):

➜ svn checkout https://github.com/qaisjp/test.git
A    test.git/branches
[...]
A    test.git/branches/test-draft-pr
A    test.git/branches/test-draft-pr/README.md
A    test.git/trunk
[...]
A    test.git/trunk/.gitmodules
A    test.git/trunk/vendor
Checked out revision 31.
➜ cd test.git/trunk/vendor
➜ ls
[nothing here]

The output for ls should look like this:

➜ ls
test-2

Upgrading the build infrastructure comes with a plethora of its own issues (in both senses of the word), and detailed discussion should probably be kept to a different issue. Some quick notes on this anyway:

qaisjp commented 5 years ago

Playing devil's advocate...

if there are so many caveats, it's probably not worth doing this.

I'm inclined to agree.

I would however say that this isn't really a problem for _new (prospective) contributors as excellent first-time developer documentation here isn't available anyway. If we used submodules we would be forced to to write a comprehensive developer guide. (Good!)

These developer guides can then be referred back to later, so it's useful for seasoned contributors as well.

The developer guide could also include git hooks that automate some of this.

it's still too complex!

Yeah... that's true. But it's cleaner and decreases cost of dependency maintenance. Just remember to use the recursive flag, you lazy git!

no, but really. it'll be a pain.

That's why I want your feedback. Please give me feedback! (Even if you're in support.)

qaisjp commented 5 years ago

We'd also need to generate our own revision numbers (git rev-list --count HEAD yields 7017, which is incorrect)... or still rely on svn checkout to generate it for us.

Actually, svn checkout currently generates this revision number: 11833. Which doesn't match up with MTA's r16616. So one of of these generated numbers also have another constant added to it.

(I recall @Cazomino05 saying this as well when we moved back to Git.)

sbx320 commented 5 years ago

I think that offset was chosen back when we moved to Github, to ensure that we get properly incrementing build revisions.

qaisjp commented 5 years ago

The build server uses git now (courtesy of ccw), and has been confirmed to pull in submodules! I am not sure if it clones recursively, but according to the internet it does. After testing, we can confirm that our build server does pull in submodules!

What's next?

This change seems to have approval from @patrikjuvonen & @saml1er (via 👍) and an implied approval by @botder (via milestoning).

We still need to evaluate clone times & get approval from @sbx320 (in response to their feedback).

Once clone times have been evaluated and confirmed to not be a blocker, I will be happy with the "approval level". Feedback or blessing from more of the dev team would be great.