DISTORTEC / distortos

object-oriented C++ RTOS for microcontrollers
https://distortos.org/
Mozilla Public License 2.0
434 stars 67 forks source link

Improve Makefile for simple targets #10

Closed jasmin-j closed 8 years ago

jasmin-j commented 8 years ago

Makefile:

Signed-off-by: Jasmin Jessich jasmin@anw.at

KamilSzczygiel commented 8 years ago

I just have one general comment here - which also applies to pull request #9. I try to follow a principle that "one commit == one logical change", so I'd really like this pull request split into a few commits, each changing one thing. For example:

  1. don't include makefile snippets or don't traverse the tree for simple targets, which also causes "make clean" not to generate output directories before removing them
  2. add configure
  3. add distclean
  4. add menuconfig
  5. add help
  6. add "unquote_d" macro
  7. check for existence of selectedConfiguration.mk

We don't have to pay for each commit, so it's perfectly fine to have a lot of them. This way all the changes can be easily verified, inspected and understood. This also makes it easier to revert any change (only if that would be needed at some point in the future) - which wouldn't be possible with your single commit changing a few things at once.

I'll put more comments in the commit in a moment.

KamilSzczygiel commented 8 years ago

Oh - one more thing - in the commit message we could try to follow the standard used by other projects. In the first line please state the general info about the change. If some more explanation is needed, put an empty line and then put the long explanation (which can span multiple lines or paragraphs). If we browse the shortlog of the project, a change with just "Makefile:" in the first line doesn't say much to the reader.

If I remember correctly, the recommended line length for commit messages is 72 characters (or sth like that).

I should add a HACKING (or CONTRIBUTING? any better name?) file to the repository with all the guidelines for contributions (possibly with the code style guide too)... I'll do that soon. I hope that my comments don't kill your interest in this project (;

jasmin-j commented 8 years ago

I try to follow a principle that "one commit == one logical change", so I'd really like this pull request split into a few commits, each changing one thing.

The problem with such changes is, that you develop them at once. It is not simple to write such make macros without testing it. In this case It would have been possible to test it with 'clean' first, but this is not always the case. Splitting it later is a lot of effort, which may reduce the "fun" of participating at an Open Source project. We do this all in our spare time, so please consider this when you argue on pull requests in the future.

Please don't get me wrong, I understand you completely and I will change it, but there needs to be rules for pull requests. And how such actions are split is a matter of taste. If you like it smaller than the person who did it and force him to make it even smaller, this can lead to people leaving the project. Please consider this in the future.

Again, in this case I follow your arguments, but this might not feasible for all persons.

This also makes it easier to revert any change (only if that would be needed at some point in the future) - which wouldn't be possible with your single commit changing a few things at once.

I agree.

KamilSzczygiel commented 8 years ago

If you want I can split the commit for you - it's not a problem.

It's actually not that hard once you have it working and committed. In such case I'd do a "mixed reset" to the previous revision and use "interactive staging" to select lines that should be committed. With this option you can select which diff "hunk" goes into the commit and which stays for next commits (you can even edit the hunks before staging them). I don't know whether you use any GIT client, but for example in EGIT (GIT plugin for Eclipse) all of that is very simple. Interactive staging can also be done with command-line git tools.

The list that I proposed is just an example - it may be a lot easier if you change the order or merge some of the steps - for example you could add the functionality of "simple targets" (which doesn't inlude any other files) as the last one (or just after adding "configure", "distclean", "help", etc.). I just wanted to show the principle, not the exact order and partitioning for this pull request.

I know that part of the problem is that I didn't state any rules in the first place, so that's why I really could do the splitting for you, but I'm not sure how would you like the fact that I would be changing your commit so much.

jasmin-j commented 8 years ago

In the first line please state the general info about the change. If some more explanation is needed ...

OK, I will do that.

I should add a HACKING (or CONTRIBUTING? any better name) file to the repository

Yes, you should do that!

I hope that my comments don't kill your interest in this project (;

No it will not! distortos is an interesting project and you are an excellent technician. Now when people are joining some rules needs to be clarified. Rom was not built in a day ;) You did some comments and I did some other. That's it. It may take some time to cope with new people but at the end you will be a good maintainer, I guess.

jasmin-j commented 8 years ago

If you want I can split the commit for you - it's not a problem.

We need to decide who does it and I would prefer to do it. I need to practice GIT ;) But I may need some time due to my daily work.

It's actually not that hard once you have it working and committed. In such case I'd do a "mixed reset" to the previous revision and use "interactive staging" to select lines that should be committed.

I will check that. Need to learn it anyway.

I don't know whether you use any GIT client, but for example in EGIT (GIT plugin for Eclipse) all of that is very simple. Interactive staging can also be done with command-line git tools.

I use vi for Makefiles ;)

I know that part of the problem is that I didn't state any rules in the first place, so that's why I really could do the splitting for you, but I'm not sure how would you like the fact that I would be changing your commit so much.

a - I like to learn the required GIT commands to do this. b - I wouldn't have any problem, if you would change my patches.

jasmin-j commented 8 years ago

There are now pull request #14 and #15. -> closed