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

Change the armips submodule to a git subtree. #10161

Closed
wants to merge 3 commits into from

Conversation

orbea
Copy link
Contributor

@orbea orbea commented Nov 19, 2017

I know my previous subtree PR was closed, but after talking about this in #ppsspp @ freenode I was given a 'Maybe' at the suggestion of changing armips into a git subtree so here is the proposal.

The main reason to do this is never have this compile issue again and help any noobs who try compiling ppsspp and may not understand that they need the submodule.

../Core/Debugger/SymbolMap.cpp:41:39: fatal
error: ext/armips/Core/Assembler.h: No such file or directory

Additional reasons include inherent issues with submodules which have been explained before and the goal of reducing the number of submodules that will be cloned with git submodule update --init --recursive.

Additionally the recursive tinyformat submodule in armips is now a git subtree after this PR and will pose no problems here. Kingcom/armips#125

This subtree was made with.

git remote add armips https://github.com/Kingcom/armips
git subtree add --prefix ext/armips armips master --squash

It can be updated at any time with.

git subtree pull --prefix ext/armips armips master --squash

Some documentation on git subtree.
https://www.atlassian.com/blog/git/alternatives-to-git-submodule-git-subtree

@hrydgard
Copy link
Owner

I'm gonna read up a bit on git subtree, seems like a feature that might be worth learning. Armips seems like a much better candidate for a potential first use of subtree than our monstrous FFMPEG repo, that's for sure.

@orbea
Copy link
Contributor Author

orbea commented Nov 19, 2017

Yea, that is a great point about FFMPEG. :)

@unknownbrackets
Copy link
Collaborator

If we're using one submodule, what's the benefit of this? We don't get blame or anything else, so it just makes this repo larger, mainly:

https://github.com/orbea/libretro-ppsspp/blame/2f0cd65236ef79ae2fabd9611c6bf77c0d951e91/ext/armips/Archs/ARM/ArmRelocator.cpp

-[Unknown]

@orbea
Copy link
Contributor Author

orbea commented Nov 19, 2017

I'm not really sure I understand what you are asking? The commits do not have to be squashed, but that would make the commit history rather messy and would inflate the size of .git/. Either way the commit range between merges is documented in the commit messages automatically and it is easy to bisect using the upstream repo as a result.

The benefits have been discussed before, but here they are again.

  1. Entirely avoids the build error for the missing ext/armips/Core/Assembler.h include.
  2. Submodules do not work well with shallow clones because the referenced commit could be at any depth.
  3. Github has a horrible habit of throttling some users to around 10 kb/s or less during some clones. This is extremely frustrating in projects with many submodules where every other submodule clones at ridiculously slow speeds and requires the user to ctrl+c and start over many times over.

This does not solve the 2nd or 3rd issue entirely, but it does help mitigate it a little, but you are right that this does inflate the size of the ppsspp repo. However users have to clone armips either way so in the end its actually smaller since armips commit history does not have to be cloned.

@hrydgard
Copy link
Owner

OK, I understand what subtree is now but I don't see many major advantages for us.

Since we are not going to do this to FFMPEG anyway, we will still have submodules, so people will still have to use git clone --recursive or git submodule update --init --recursive after cloning.

There will therefore be no user experience benefit - closing.

@hrydgard hrydgard closed this Nov 20, 2017
@orbea
Copy link
Contributor Author

orbea commented Nov 20, 2017

Sorry to hear that. Even without ffmpeg there is user benefit for every submodule they do not have to clone separately from the main repo, but its your choice. Would you mind updated the armips submodule so that at least tinyformat does not have to be cloned as a submodule?

@hrydgard
Copy link
Owner

Yeah, I'll do that.

hrydgard added a commit that referenced this pull request Nov 20, 2017
@orbea
Copy link
Contributor Author

orbea commented Nov 20, 2017

Cheers!

@orbea orbea deleted the armips branch November 20, 2017 18:21
unknownbrackets added a commit that referenced this pull request Nov 21, 2017
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.

3 participants