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

Fix docs missings & improve dev scripts #1981

Merged
merged 11 commits into from
May 23, 2013
Merged

Fix docs missings & improve dev scripts #1981

merged 11 commits into from
May 23, 2013

Conversation

LeoColomb
Copy link
Member

  • Move installation guide of specific platform in INSTALL.md instead of readme.txt
  • Leave all docs guides in docs folder
  • ...

@bilderbuchi
Copy link
Member

👍 to the gitignore changes.

@LeoColomb
Copy link
Member Author

+1 to the gitignore changes.

@bilderbuchi Also, there are ~20 .gitignore into the openFrameworks repo. Is it not better if we mix/merge some of them in the main .gitignore, to clean directories, save place, avoid mis-understandable hierarchy and make easier to keep it up to date?


@arturoc What do think about this PR?

@bilderbuchi
Copy link
Member

@bilderbuchi Also, there are 17 .gitignore into the openFrameworks repo. Is it not better if we mix/merge some of them in the main .gitignore, to clean directories, save place, avoid mis-understandable hierarchy and make easier to keep it up to date?

this is only partly right - the "important" ones are only 4 - .gitignore, apps/.gitignore, addons/.gitignore, examples/.gitignore, and they are distributed like that to precisely avoid one huge gitignore file with a lot of special rules to only affect subpaths etc. Instead we have all the general ignore patterns in the root .gitignore file, and the subpath-relevant (e.g. for "official" addons) in the respective subpath.

most of the others are necessary placeholders to be able to check empty folders into the git repo (all thos with "Ignore everything in here apart from the .gitignore file" in them), or ignore files for specialist applications (e.g. /dev/style/) for which it would make no sense to pollute/bloat the more general gitignore files higher up in the tree.

@arturoc
Copy link
Member

arturoc commented May 16, 2013

looks great to me, are really there so many gitignores? in that case we should remove some but i think having gitignores in important folders like addons, examples and apps can be useful

@arturoc
Copy link
Member

arturoc commented May 16, 2013

didn't read this before:

most of the others are necessary placeholders to be able to check empty folders into the git repo (all thos with "Ignore everything in here apart from the .gitignore file" in them), or ignore files for specialist applications (e.g. /dev/style/) for which it would make no sense to pollute/bloat the more general gitignore files higher up in the tree.

to check empty folders we switched to a .gitkeep file in examples that's better cause that way we avoid ignoring the contents of that folder, if there's any folder with .gitignore still it should be changed to .gitkeep

@bilderbuchi
Copy link
Member

but sometimes you want to have empty folders in the repo, which stay empty (i.e. ignore everything except gitignore). gitkeep does not do this. example: the gitignores in the libs folders.

@bilderbuchi
Copy link
Member

that way we avoid ignoring the contents of that folder

that does not always make sense: if we wanted to not ignore content in that folder, the content would be in there and we wouldn't need a gitkeep. if not, the content is user-specific or autogenerated, and thus should not be in the repo. thus, the gitignore which ignores everything except itself.

gitkeep makes sense where we want to show users that e.g. there's a /data folder, but we don't use it for anything (i.e. the example doesn't put anything there), but without a dummy file in it we could not commit it as git does not see empty directories.

@bilderbuchi
Copy link
Member

the one which could be optimized away would be the one in SerialTest.
apps/devApps/VboExample/bin/data/.gitignore could be renamed .gitkeep cause it's empty

the ones in apps/devApps/AdvancedImageLoading and /libs/openFrameworksCompiled/lib/*/ could probably be optimized away, but you would have to replace them with gitkeep files, and some rules in the upper gitignores to not ignore those gitkeep files. number of files stays the same, number of rules grow, i'm not sure it would be worth it, since the current gitignore files are pretty explicit about what they do. if you wonder why you can't commit files which are in there, there's a gitignore file right there telling you why, instead of some special path-adapted rule in another gitignore 3 levels up the file tree.

@LeoColomb
Copy link
Member Author

the one which could be optimized away would be the one in SerialTest.

OK. Stupid question: did we need to keep the .elf & .hex files in serialTest?

@LeoColomb
Copy link
Member Author

@bilderbuchi @arturoc OK, so I tried something "just for fun", see my latest commits. What do you think about this?
If it's too bad, I revert it and I don't change anything.

@bilderbuchi
Copy link
Member

I'm sorry, I have to ask, how familiar are you with the gitignore logic?
Did you rigorously test your changes? Are you aware that files which are already in the repo are unaffected by gitignore rules? This means you have to create a lot of new mock files to test if your rule changes work properly. Are you aware of gitignore parsing order (i.e. ignore files deeper in the file tree override ones higher up)? btw, were you able to commit this without using the force/-f flag?

It's unfortunate that you put all your changes into one commit, this way it's really hard to isolate the good/neutral changes from the bad. There are some things in there which are ok and make sense. There are some redundant changes in there. There are some things which I think are wrong. Unfortunately I currently can't test this rigorously.

Also, these changes have nothing to do with this PR, but will keep it from being merged until this is resolved. A better way (for the future) is to submit a separate PR for a separate topic.

Considering how complicated it is/was to arrive at a working gitignore system (and this doesn't even consider people force-committing things improperly instead of correcting rules), I vote for removing these two commits. Actually, I'd prefer you to rebase them out of the PR. If you only revert, your commits/changes will pollute the git history (as you will show up as the last committer). Unfortunately, this could mean bad things for whoever pulled your branch in the meantime - another reason to follow our contribution policy and keep PR topics separated (especially if they're "just for fun").

In any case, I downloaded the patch files of your commits, so the work is not lost. Hopefully I'll get around to teasing out the good changes sometime, next time I streamline the gitignore structure.

@LeoColomb
Copy link
Member Author

Did you rigorously test your changes? Are you aware that files which are already in the repo are unaffected by gitignore rules? This means you have to create a lot of new mock files to test if your rule changes work properly. Are you aware of gitignore parsing order (i.e. ignore files deeper in the file tree override ones higher up)? btw, were you able to commit this without using the force/-f flag?

Yes, I do. I have tested it about 1 hours, make modifications in files, create new ones and new folder, modify hierarchy, ..., and try to commit all of this changes separately, and never use -force or something else to overwrite ignore' rules.
My conclusion is: it works great, but only one point isn't resolved yet: changes in official addons can't be committed without -force. Please note that is _exactly_ the same situation as before those changes into gitignore files.

It's unfortunate that you put all your changes into one commit, this way it's really hard to isolate the good/neutral changes from the bad.

It's true, I'm sorry.

Also, these changes have nothing to do with this PR, but will keep it from being merged until this is resolved. A better way (for the future) is to submit a separate PR for a separate topic.

Right, I'm sorry. I will close this PR and create the same but without latest ignore diffs.

@LeoColomb
Copy link
Member Author

Edit: In fact, no problem in official addons.

@bilderbuchi
Copy link
Member

OK, thank you.

changes in official addons can't be committed without -force. Please note that is exactly the same situation as before those changes into gitignore files.

Are you sure? This is not correct afaik - I just tried and can edit e.g. ofxOsc without problems, and git will pick it up. Although I just realized that ofxSynth and ofxThreadedImageLoader are missing in the list of official addons.
edit: ah i see.

You know what, I'll open a new issue for the gitignore changes (edit: #2057), and we'll move the discussion over theree, collect all the problems, and then I'll make a PR based on your work.

@LeoColomb
Copy link
Member Author

@bilderbuchi Rebasing succeeded. So, good to merge?

@bilderbuchi
Copy link
Member

looks good to me, but I can't judge the packaging script, but @arturoc seems to have already approved the changes?

@LeoColomb
Copy link
Member Author

Yes => here
Also, do you want I make a PR with my .gitignore changes?

@bilderbuchi
Copy link
Member

Also, do you want I make a PR with my .gitignore changes?

nah, I'll do that, I got your commits. some other stuff to fix anyway.

@LeoColomb
Copy link
Member Author

@arturoc so finally, is that good?

@arturoc
Copy link
Member

arturoc commented May 23, 2013

yes seems good to me

@bilderbuchi
Copy link
Member

@arturoc so will you merge it, or should I do it?

arturoc added a commit that referenced this pull request May 23, 2013
Fix docs missings & improve dev scripts
@arturoc arturoc merged commit 9b28735 into openframeworks:develop May 23, 2013
@LeoColomb LeoColomb deleted the Fix-Docs branch May 23, 2013 18:23
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