Skip to content

Commit

Permalink
xenial (ubuntu:16.04) needed, trusty (ubuntu:14.04 has no gcc-5)
Browse files Browse the repository at this point in the history
  • Loading branch information
@LynxAbraxas
LynxAbraxas committed Oct 8, 2018
1 parent 995cd6d commit feb5a61
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion Dockerfile
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
################################################################################
# builder
################################################################################
FROM ubuntu:14.04 as builder
FROM ubuntu:16.04 as builder

RUN apt-get update && apt-get install -y --no-install-recommends \
libsdl1.2-dev libsdl-mixer1.2-dev libsdl-image1.2-dev byacc gtk+-2.0-dev gcc-5 g++-5
Expand Down

4 comments on commit feb5a61

@DEVoytas
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could rebase/reorganize commits so this kind of changes are squashed so we do not have so many intermediate commits that may not work ?

@LynxAbraxas
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not well displayed in GH' PR view, but if you have a look at the commit graph (network view, or in gitk) you will see that all execept the last commit are on an orphan branch, i.e. I merged ctp2DF with --allow-unrelated-histories in order to preserve all tests (and comments) that did not work without the then following changes. E.g. in this specific commit, up-grading the OS for a newer gcc.

@DEVoytas
Copy link
Contributor

@DEVoytas DEVoytas commented on feb5a61 Nov 2, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I realized GH cannot handle displaying those properly for some reason, but I used gitk locally :)

My point was that we should try to no introduce commits that do not provide working code (i.e. are broken in some way: do not compile for example). This is considered good practice in order to be git bisect friendly and also with workflows that allow fast-forward merging (not our case), so no commit on target branch is broken.

To be clear: I am advocate of small incremental commits (they are much easier to review then big changes squashed into one giant commit). But I just think that all commits should be usable and useful (i.e. should work and provide some improvement). I found however there is another approach to that: http://alblue.bandlem.com/2013/08/should-every-commit-compile.html

For this specific case my guess is that, by leaving this commit in the history you wanted to show that ubuntu:14.04 did not work, and then, after changing it to ubuntu:16.04 it finally worked.
But let's notice that choosing initial ubuntu:14.04 is the arbitrary one.
Let say someone started from 12.04 and it did not work, so he tried 13.04, 14.04, and so on...
Would we want to see all this trial and error in the commit history? IMHO, no.
Most of the time we are not interested what did not work, but what finally worked.

So my approach usually is, that before pushing I try reorganize my commit history to have minimum working version as the first commit, and some improvements/optimizations/extra features as subsequent commits.

I realize, that there are different approaches and not everyone will agree with that. I just wanted to share my point of view and clarify my initial comment :)

EDIT: another reason to consider it come up here: c5e0913#commitcomment-31152084

@LynxAbraxas
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But let's notice that choosing initial ubuntu:14.04 is the arbitrary one.

No, it's the oldest still available officially ;-)

Would we want to see all this trial and error in the commit history?

I definitely do (if the trail and error is properly commented), because such commits have saved me from loosing a lot of time trying something that was already tested to fail. A clean-up of history is likely a removal of significant information. If you are just interested in the changes over a view commits, git can show you just that. I agree, that ideally all merge commits to master should compile, though this is not guaranteed for those that have been made before (while still committing to SVN).

Anyway, I squashed all that is needed on GH (removing .gitlab-ci.yml) into one commit and created #23 as an alternative to #13.

Please sign in to comment.