3421: Makefile improvements #813
Conversation
https://tech.davis-hansson.com/p/make/ Prevents things like silent shell pipeline errors in recipes, unexpected behavior from undefined variables, etc.
AFAIK, prefixing a target with a `.` (aside from the special targets make itself treats differently) is just to exclude it from being the default target, but the default target is already defined above this one. Also, it just runs another target which doesn't exist AFAIKT.
This is opinionated, so I understand if community members disagree and I'm happy to back this out. It's conventional (and I prefer) to group variables toward the top of the `./Makefile` so I've done that. I also prefer separating task-oriented "phony" targets above any "real" targets (whose recipes create/update actual build artifacts on the filesystem). The task-oriented targets tend to be a better starting point for any developers approaching the `./Makefile` to understand where to get started.
From context, I'm assuming this is just an omission.
I also prefer having a `.PHONY: ...` declaration for each phony target, as is done in the rest of the `./Makefile`, because it makes is easier when removing or refactoring targets not to forget to also remove or adjust the `.PHONY: ...` declaration.
Fully parallelize the build of the environments since they tend to be network I/O bound. Parallelize the run of tests to use all CPU cores.
I don't see anything in here that I can find references to elsewhere and we're certainly running test coverage reports in tox and on CI now.
On systems where the default Python is Python 3 (such as on recent Debian/Ubuntu versions), then `$ tox -e codechecks` has a ton of failures related to Python 3 compatibility. This explicitly forces it to use Python 2.7 until we have Python 3 compatibility.
Codecov Report
@@ Coverage Diff @@
## master #813 +/- ##
========================================
+ Coverage 92% 93% +1%
========================================
Files 155 155
Lines 27399 29231 +1832
Branches 4107 4687 +580
========================================
+ Hits 25177 27070 +1893
+ Misses 1538 1497 -41
+ Partials 684 664 -20
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thank you, and welcome, @rpatterson!
I am not opposed to the changes to Makefile. But I too have never used make when working on Tahoe-LAFS. I would like to hear everyone's thoughts.
I am also not opposed to structuring commit messages in any way you like. I don't think Tahoe doesn't have any documented conventions around these things, and a project of Tahoe's age and history is going to have commit messages in a variety of styles. I think I will just accept that as a fact of life. :-)
I was wearing the reviewer hat last week (admittedly didn't do a good job of it), but I was also unsure if I should go ahead and approve before there's some consensus about this. Let's wait for hearing from more people?
|
I also personally tend to encode shell-lines into Makefiles. I don't make heavy use of the Makefile in this project, but some cleanups are certainly a good idea (since there's already a Makefile anyway). As to the commits, the actually commit message have mostly been personal taste. Some developers in the past where also very fond of Now the workflow has become to make branches and then those branches get merged. So, there could be some value in putting structure into the actual merge commits -- I'm a bit less sold on having every individual commit follow such a pattern (but of course, anyone may do so if they prefer). Probably a good way to move forward on that topic would be to propose a PR with some developer conventions etc written down and we can then discuss which ones we like, etc. and so then anything with consensus around it can make it into that document (instead of just being in some random PR discussion). I thought @exarkun had written down conventions for "news" file entries, but I may be confusing with another project (because I don't see such a document here). You should be able to use |
There was a problem hiding this comment.
Besides @sajith 's comment and the codechecks env thing, looks good.
The "news" wording seems a little awkward. Maybe "Makefile cleanups for developers" or similar? Mostly, the NEWS should be addressing users (not developers) directly. So this could just be an empty 3421.minor instead.
K, I'll consider that a separate issue for now since so far everyone seems not to mind if I keep using conventional commits in my individual commits. Someone LMK if I should draft such a document though I imagine that, being so green, I may not be the best candidate. I'm always happy to write down opinions, however. ;-)
I'm not clear if you're asking for revision here or just commenting on the general issue. I did add a news file entry and I see it included in the |
|
I would at least re-phrase the news item, or maybe just make it There's also the comment about "bash" from @sajith and question about why we need a Thanks! |
|
Yeah, I'll follow up on your other comment shortly, @meejah.
Ah, thanks! I couldn't tell the difference between |
This includes only developer-oriented changes.
|
I think the difference is that .minor doesn't make the news, and .other ends up in the list of links? These files are consumed by https://github.com/twisted/towncrier and |
Yeah, I reviewed I'll merge once CI is green. |
Trac ticket
This includes some (IMO)
./Makefileimprovements. All but one of the commits are changes that are more "correct" and one brings some opinion of mine into the mix.Until I can meet the team/community and sync up on how we prefer to work on this project, I thought I might use the PR to kick off some discussion.
First of all, I've been practicing Conventional Commits for the last year or so and I'm pretty happy with it. I do't see any particular convention demonstrated in the existing commits in this project. Any objections to me continuing to use Conventional Commits here? (No problem if no.)
As @exarkun mentioned in IRC, he hasn't ever used the
./Makefilein this project which indicates it may be mostly dead code.<preach>At any rate, I do personally prefer to use./Makefilefiles primarily to accomplish what docs with a series of shell commands to copy and paste intend to accomplish. I find having them in executable form is not only more convenient for developers but tends to prevent the docs going stale as developers are much more likely to be using it and finding out when it's gone out of date. I also find that if the./Makefileis well commented and minimizes the use ofmakemagic, that it's not significantly less approachable for junior developers. I also like to use the./Makefileas a way to capture and document common developer tasks (e.g. how to run tests under a debugger, how to bump the version, etc.). As to whymake, I find it more discoverable and maintainable than a collection of shell scripts in most cases (I still use separate scripts for more complicated things). It is also the only thing I'm aware of that provides some sort of incremental build system that is nearly universally available and incrementalism is important for local development so that minor per-instance delays don't compound over repetition to waste significant developer time.</preach>Any objections to me continuing to use the./Makefilethis way? (Again, no problem if no, I can hide my./Makefileusage.)