Simple.SrcDist cleanup (fixes #3656)

[fmaste]

• The PackageDescription is not changed any more, just the pieces of it we need to (see preprocessComponentSdist).
• Created filterAutogenModules and filterAutogenBuildInfo functions so given a PackageDescription and BuildInfo returns filtered answers. Now is clear where to add an autogen-c-sources field.
• Created sdistSourcesList and sdistSourcesDirectory functions moving the logic out of frontend sdist where this last one just reads the flags and prints messages.
• findSetupFile, findMainExeFile and two pieces of code from listPackageSources where identical, extracted all on findMainFile (You can see through the commits how it is done step by step).
• Renamed not exported functions so now we have findModulesFiles, findMainFile, findIncludeFile and findSetupFile together.
• TODO: verbosity variable of sdistSourcesList, sdistSourcesDirectory goes to listPackageSources then listPackageSourcesOrdinary that calls defaultPackageDesc verbosity that ignores it. But listPackageSources is exported and is too much for a single PR.

There are a lot of commits because it's the only way to make small steps to figure all this out and test between every change. Dont worry I will merge --squash! but before you can see that I tried hard not to break anything.

[23Skidoo]

I don't think we should tell people what their workflow should be, as long as it doesn't interfere with other people working on the project ;)

Spurious merge commits pollute history and make it harder to do code review, that's why we care about them.

[ezyang]

Yes, but @fmaste has already stated that he will merge squash before pushing to master (IIUC, that will be a merge, then a git rebase -i which will result in a single commit with no merges) so no problem right :)

On September 16, 2016 6:40:14 PM GMT+09:00, Mikhail Glushenkov notifications@github.com wrote:

I don't think we should tell people what their workflow should be, as
long as it doesn't interfere with other people working on the project
;)

Spurious merge commits pollute history and make it harder to do code
review, that's why we care about them.

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#3702 (comment)

Sent from my Android device with K-9 Mail. Please excuse my brevity.

[23Skidoo]

Yeah, it's no problem in this case, but in general merge commits make it harder to do code review, so it's nice to get rid of them before submitting a PR.

[ezyang]

No, I don't see how merge commits necessarily cause problems at review time. For example, GitHub suppresses the diff if the merge is "clean". Very rarely git will mess up an "obvious" textual merge. But that's what looking at the final diff is for.

If the merge is dirty, well, it is true that it is "easier" to review patches if the various bits of the merge have been backported back. I do this for my own patchsets. But it is extremely tedious and time consuming. I wouldn't wish it on anyone else. For example, for this patchset, I reviewed it by just clicking on "Changed files" overall and looking at the changes.

[fmaste]

You certainly can! :) There are at least two ways I can think of ...

Everything is possible with git but no thanks, sounds like a recipe for disaster

@23Skidoo @BardurArantsson, I don't think we should tell people what their workflow should be, as long as it doesn't interfere with other people working on the project ;)

As I'm doing some refactoring I do little non-destructive changes with a silly commit description just to identify them when looking at the history. At all time I test locally and if something fails I cherry pick and correct.

Yeah, it's no problem in this case, but in general merge commits make it harder to do code review, so it's nice to get rid of them before submitting a PR.

Nobody else is working on this branch and the merge is not shown on the diff. After squashing all to master the result will be the same as rebasing and I can delete this branch. The only problem I see is that you have to skip merges if you are looking at every commit separately, but I doubt somebody is doing that with all this commits

[ezyang]

@fmaste You're all good. Squash and push!

[23Skidoo]

@ezyang

For example, GitHub suppresses the diff if the merge is "clean".

It only recently started doing that. Not sure how well it works for "dirty" merges.

I do this for my own patchsets. But it is extremely tedious and time consuming. I wouldn't wish it on anyone else.

I sympathise, but this is essentially an argument for not removing merge commits at all.

For example, for this patchset, I reviewed it by just clicking on "Changed files" overall and looking at the changes.

It works in this case, but in general not having everything rolled up in one giant commit and using logical commits instead of historical makes the reviewer's life easier.

[23Skidoo]

@fmaste Are you going to update this PR? This is basically good to go, just needs to be squashed and have some minor conflicts resolved. Anything we can do to help?

[ezyang]

@23Skidoo Maybe we should just squash and push ourselves.

[phadej]

I assign this to @ezyang to squash and push

[phadej]

This PR contains some good stuff, but it cannot be merged as is. v2-sdist doesn't (and will not) run configure beforehand.

[phadej]

sdist code have changed, and some changes are there now.