Several fixes, in separate commits

[th-otto]

• Allow prefix for cross-tools to be overridden
• Fix "make clean" to remove objects in sub-directories
• Clean up assembler code to use % prefix for register names
• Simplify aes/vdi entry points
• Fix dereferences of type-punned pointers
• Fix filemodes

[mikrosk]

Just a suggestion, maybe "Clean up assembler code to use % prefix for register names" should be a separate PR.

Also, some of the fixes are already present in #1

[th-otto]

maybe "Clean up assembler code to use % prefix for register names" should be a separate PR.

Can't you just cherry-pick that commit when merging? That would be easier, since at least one the other commits patches the same source files.

Also, some of the fixes are already present in #1

Yes, sorry. Only noticed that when i was already done with it.

[mikrosk]

Sure, cherry pick works, it's just not as easy as pushing the "merge pull request" button. :-P (now I think about it, I'm not sure how to do this easily at all -- there is some obscure way how to get git url of this PR but it's really not meant to be used like that).

Anyway, it's up to @Landemarre, @arnaudbercegeay or @atic-atac to decide on this I guess.

[th-otto]

But it is really important to have a separate PR then? When merged, there will be separate commit for it, no matter where it came from.

PS.: looking at the travis log, i see something like
CC=m68k-atari-mint-gcc
m68k-atari-mint-gcc --version
at the very top. Where does that come from? I don't see it anywhere in the scripts. And it is called before the cross-tools are installed, so the invocation at least does not work

[mikrosk]

PR: but if you merge it via the web ui, you can't (shouldn't) mess with commits anymore, it's pushed.

CC: Yeah, that's some kind of Travis feature, to show you your current environment (as I set CC to m68k-atari-mint-gcc in .travis.yaml) but as you have noticed, not exactly useful in this case.

[th-otto]

Oh, does that come from the compiler: entry? Then i suggest to remove that, the compiler should be figured out by the configure script when cross-compiling. Also, only setting CC is nonsense when compiling libraries, you will also have to set at least AR and RANLIB accordingly

PR: but if you merge it via the web ui, you can't (shouldn't) mess with commits anymore, it's pushed.

What i mean is, there is no difference in the commits of the master branch once you have merged both PRs. It would only make a difference when you want to omit one of the commits when merging.
BTW that concept of only being able to merge commits from seperate branches has some lots of drawbacks. Ie. the history of my fork currently looks like this. This is complete mess.

[mikrosk]

I may remember that wrong but I think when I didn't set it, Travis forced its own value there (gcc vs. clang), i.e. making the situation even worse because then our projects (freemint, mintlib etc) refused to compile.

PR: I totally get it but I had a different situation in mind, in case that people wont like the transition to %% prefixes. In that case you'd end up with a tree with all your commits (incl. the prefix one) and you would have to either revert it or force-push without it.

Btw the merges: it doesn't have to be like this. Maintainer has an option to use 'rebase', i.e. first merge master back to PR, rebase the change on top of it and fast-forward it back to master. The approach is discussed here: freemint/freemint#26.

[th-otto]

In the travis log i just noticed:

../inline.rsh:7:20: fatal error: portab.h: No such file or directory
compilation terminated.
make[3]: *** [userdef.o] Error 1

Shame on me, this is due to an include portab.h at the top that file, generated by ORCS. i was quite sure that such a file exists for mintlib/gemlib, but apparently this isn't the case anymore. Will fix that soon.

But there is also a problem with the build script, it nevertheless writes:

The command "sh ./.travis/build.sh "${PWD}/.travis/tmp" "${PWD}/.travis/out"" exited with 0.
after_success
...
Done. Your build exited with 0.

[th-otto]

Regarding the commits: you would still be able to revert single commits if need be. Only because they all come from the same branch, does not mean they are treated like a single commit (unless you squash them together when merging). I also don't think that is a matter of taste to use % prefixes or not, without them you won't ever be able to compile it with newer versions of gcc. And since some of them also appear in public header files, that is not only a matter which compiler is used to compile the library, but also which compiler the user uses to compile her application. So actually, you could consider that as bug fix.
@atic-atac any opinion to what Miro suggested? I can do that, eg, put that commit on a separate branch, but then i have to remove this branch, and recreate it, since i can't remove that commit anymore.

[mikrosk]

I also don't think that is a matter of taste to use % prefixes or not, without them you won't ever be able to compile it with newer versions of gcc.

Well, the ELF flavour of it. That's the basic question here, if everyone agrees the ELF 'middle man' is the future, you're right.

Thanks for bringing the travis issues to my attention, the script is indeed buggy: calling make CROSS=yes and then make CROSS=yes PREFIX="${TMP}" install without &&! Will fix it.

[th-otto]

Well, the ELF flavour of it.

Not only. Sooner or later this will be mandatory for all targets. And with gcc 7.x (or actually also some older versions) this is already mandatory for inline asm, because you otherwise have conflicts with positional parameters. And inline asm is used a lot in all those trap_xxx macros.

[mikrosk]

Needs cleanup, one functionality per PR.

[th-otto]

Needs cleanup, one functionality per PR.

That does not work with the PR system. We had that already. If i put every commit on a separate branch, each commit has to be rebased against master, or the PR does not merge. But if you then merge the first PR, others will not apply anymore, since they affect the same files (not all of them, but some).

If at all possible, just think of each commit as being a separate PR. Once merged, they will appear as separate commits anyway in the history.

[mikrosk]

If i put every commit on a separate branch, each commit has to be rebased against master, or the PR does not merge. But if you then merge the first PR, others will not apply anymore, since they affect the same files (not all of them, but some).

Not necessarily true. Don't tell me that 'make clean' fixes collide with the register prefix changes. PR doesn't merge only in case of a conflict (changing the same lines), so changing the same file is totally fine. git is quite smart.

If at all possible, just think of each commit as being a separate PR. Once merged, they will appear as separate commits anyway in the history.

This is not the point of a PR (or a merge commit in general) -- you want to be able to step back and bisect between merge commits (PRs), not the branches they are merging. If it helps, look at the PR as a separate patch: https://www.kernel.org/doc/html/v4.12/process/submitting-patches.html#split-changes -- it's totally OK to say "this one depends on that one" but IMHO it should have a good reason.

Anyway, I'm just providing hints, to increase the chance of merging your stuff, I'm not saying all I write here is universal truth. :) But the truth is that there's 175 files changed, labeled as "several fixes" and if there's a regression, the person who will be doing bisection will certainly love you. :)

[th-otto]

Don't tell me that 'make clean' fixes collide with the register prefix changes

No of course not. Sure that could be put on a separate PR. But see my problem here: i make up a branch for it, add that commit, then make a PR for it. Then i switch back to master, make a new branch for implementing the next patch. Now i have a clean branch again, but without the fix from the other branch...

you want to be able to step back and bisect between merge commits

But you can still do that. They are still separate commits. I don't see a problem here.

But the truth is that there's 175 files changed

Most of them from the one that fixes the filemodes only. I did not change any content there, only removed the executable bit from source files.

[mikrosk]

But see my problem here: i make up a branch for it, add that commit, then make a PR for it. Then i switch back to master, make a new branch for implementing the next patch. Now i have a clean branch again, but without the fix from the other branch...

But nobody stops you from creating your own branch with everything. You are even free to use your current (PR-1) branch for whatever you want. Just create spin-offs from master for each functionality: make changes, register prefix changes, vdi/aes changes, file mode changes, ... you don't have to even look at them anymore, just keep them in origin until merged. Cherry-pick is your friend. ;)

But you can still do that. They are still separate commits. I don't see a problem here.
Most of them from the one that fixes the filemodes only. I did not change any content there, only removed the executable bit from source files.

Exactly my point. You want the maintainer to look at a PR, make him realise there's only 175 changed file modes, nothing else, boom, committed. Now there's the prefixes, vdi changes, pointer changes, ... if he doesn't like some of it, then what? Your whole set is blocked and nothing gets committed. Even if you correct it, he AGAIN has to look at all of the 175 file changes because he can't know what commits you have force-pushed, changed or added. See, it quite makes sense to separate things.

[th-otto]

On Mittwoch, 1. November 2017 04:22:11 CET Miro Kropacek wrote: Cherry-pick is your friend. ;)

I don't think so. cherry-picking will create a new object in the history, with the same changes. And when there are merge conflicts, it will create another extra merge commit. And it will make keeping track of which commits are applied in which branch almost imposssible. How am i supposed to know which commits i have already cherry-picked, when getting back to that branch a few weeks later?

If he doesn't like some of it, then what? Your whole set is blocked

Well thats a point of course. Isn't there a way to "cherry-pick" the commits from the PR when merging? ie only merge those where he sees no problem, and reject the others?

[mikrosk]

How am i supposed to know which commits i have already cherry-picked, when getting back to that branch a few weeks later?

Perhaps others can share their experience, what I usually do is:

• do development on some branch, committing whatever I like
• realise I could make a few PRs out of it
• branch off master for each change, creating N branches
• cherry pick related commits into given branch (sometimes rebase afterwards to clean it up)
• push & create PRs
• forget about it, do development as usual
• repeat until I'm not done with my dev/feature branch
  ...
• after some time (usually when I want to start a new feature branch) I sync with freemint's master (reset my new branch there) and start again. Sure, sometimes the PRs are still pending, in that case I branch off my new feature branch from the old one, if I need features from the previous one but then again, if I feel I could create a PR out of it, I repeat the steps above, always branching from the upstream master for PRs+cherry pick. Maybe not the most efficient approach but certainly doesn't lead to PRs with hundreds of changes.

Isn't there a way to "cherry-pick" the commits from the PR when merging? ie only merge those where he sees no problem, and reject the others?

Not that I am aware of.

[mikrosk]

@th-otto I suggest to keep "Clean up assembler code to use % prefix for register names" commit as a separate PR (same as freemint/mintlib#28 is) before it's 100% agreed and commit the rest.

[th-otto]

Applied now, except for the register names patch. I'll open an issue for it, since the original patch does not apply anymore