Skip to content

Conversation

@wilzbach
Copy link
Contributor

The patching done by the auto-tester makes edits to the win64.mak files nearly impossible, see e.g. the famous dlang/phobos#2526 (oldest non-merged PR at Phobos) or more recently dlang/phobos#5940.

Hence, I propose to check for a NO_AUTOTESTER_PATCHING_V${diffVersion}_${repoName} string, s.t. we can gradually remove these patches.
Should it become necessary to introduce patching again in an unlucky future, you can simply bump diffVersion.

@braddr
Copy link
Owner

braddr commented Dec 19, 2017

This is one option. The underlying issue(s) need to be tackled, and a number can probably be directly addressed. As far as the referenced pr's go, I still object to parts of them but would have to re-review to recall details. Regardless, I'm about to leave town for a week and have a rule to not make changes that aren't absolutely necessary before traveling. So, this will all wait until after returning.

@wilzbach
Copy link
Contributor Author

This is one option.

Yep. You don't like it? Do you prefer something else?
Also should we do all three repos at once or just try Phobos and add the similar procedure in a follow-up?
(I will need to modify all three win64.mak files soon as we want to move away from the win64.mak files and drop support for DigitalMars make)

As far as the referenced pr's go, I still object to parts of them but would have to re-review to recall details.

Fair enough, but that's independent to this PR (i.e. the process of removing the patching).

Regardless, I'm about to leave town for a week and have a rule to not make changes that aren't absolutely necessary before traveling. So, this will all wait until after returning.

I hope you had pleasant Christmas & holidays!

@wilzbach
Copy link
Contributor Author

Ping @braddr - so what's your opinion on this? I would love to start moving away from the win32+64.mak files...

@wilzbach
Copy link
Contributor Author

Ping. If you don't like grabbing for a string. How about a dedicated file or do you have something else in mind?

@wilzbach
Copy link
Contributor Author

Friendly ping about this ;-)

@andralex
Copy link
Contributor

ping @braddr can you please help us unclog this

@braddr
Copy link
Owner

braddr commented Jan 30, 2018

Sorry for taking so long to get back to looking at this. Re-reviewing the changes, the problem I have is that it looks like this strictly providing an off switch that it looks for in the win64.mak file for each repo. I'm not sure I see how that facilitates gradual anything. I see how it facilitates ONE step, which might be enough. What am I missing?

As an aside, cut out the hyperbole, it's not almost impossible to edit the make file, as is manifestly obvious by the number of changes it's had over the years.

@wilzbach
Copy link
Contributor Author

I'm not sure I see how that facilitates gradual anything. I see how it facilitates ONE step, which might be enough. What am I missing?

Well how else would you do it?
I want to move forward and unify win32.mak, win64.mak and posix.mak into one file and that requires that I can make changes which conflict with the diffs you have.
My approach here was: "set a flag which disabled the diff, s.t. I can remove the diff"
I'm more than open to better suggestions.

@wilzbach
Copy link
Contributor Author

Ping @braddr - so how can we move forward here?

@marler8997
Copy link

The simple solution would be to include @braddr's patches in the repo right?

@wilzbach
Copy link
Contributor Author

The simple solution would be to include @braddr's patches in the repo right?

We can't do that, because it still results in merge conflicts as the patch can't be applied.

Also AFAICT @braddr doesn't want to open 10 pull requests (for each repo x master and stable) whenever he makes changes to the auto-tester infrastructure, so even if we do a collaborative effort to get rid of all these patch files and coordinate it with @braddr, he might still need to add new ones in half a year later
Of course, including the patchs and removing them entirely would be my preferred way too...

@marler8997
Copy link

Sounds like we've painted ourselves into a corner :)

@wilzbach
Copy link
Contributor Author

wilzbach commented Mar 1, 2018

Sounds like we've painted ourselves into a corner :)

Yes, but I still have hopes that we can find a way to remove the patching by auto-tester.
@braddr it seems like you don't like the idea proposed here. What would be your preferred way of progressing with this?

@rainers
Copy link

rainers commented Mar 2, 2018

@braddr see https://github.com/dlang/dmd/pull/7968/files#diff-180360612c6b8c4ed830919bbb4dd459R56 for an example how to build and run the test suite with compiler overrides without patching the makefiles or d_do_test. We can also add targets to the makefiles to simplify that for the auto-tester.

@wilzbach
Copy link
Contributor Author

We can also add targets to the makefiles to simplify that for the auto-tester.

Yeah and it would be a lot easier to do with if we have a way to disable the patching within the PR.

@braddr it seems like you don't like the idea proposed here. What would be your preferred way of progressing with this?

Friendly ping.

@wilzbach
Copy link
Contributor Author

@braddr it seems like you don't like the idea proposed here. What would be your preferred way of progressing with this?

Friendly ping.

pong.

@wilzbach
Copy link
Contributor Author

Another PR that is lingering in the queue due to the patching errors is dlang/dmd#7414

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants