Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Revert "st: copy config file in 'prePatch' instead of 'preBuild'" #85775

Merged
merged 1 commit into from
Apr 29, 2020
Merged

Revert "st: copy config file in 'prePatch' instead of 'preBuild'" #85775

merged 1 commit into from
Apr 29, 2020
+2 −3

Conversation

oxzi
Copy link
Member

@oxzi oxzi commented Apr 22, 2020

If I'm not mistaking, this previously merged PR has broken common usage and the compatibility of patches.

The following was my argumentation from the PR #85443, as @Mic92 recommended me to create a new PR.

I'm not quite sure about this changes, but I think it is breaking with common usage. I would guess that most users are adding some provided patches from upstream and a custom configuration file.

By default, the patches were written against the vanilla release version. Thus, by applying the patches first, they should work. The custom configuration file will then just replace the (potentially patched) config file.

However, if the custom configuration is applied at first, some patches are not compatible anymore because they are also adjusting the configuration file. This obviously fails, because it is no longer in its vanilla state.

Furthermore, I think the argumentation is wrong. Afaik, the patch phase is run before the build phase, not afterwards. The explained state in the commit message was already there before.

In a nutshell, this reverts that patches need to be compatible with the user's custom configuration. The user config will override the previously patched default configuration.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Apr 22, 2020
@FRidh
Copy link
Member

FRidh commented Apr 22, 2020

When modifying files, either use patches or postPatch. prePatch can indeed be problematic because it can cause patches to fail to apply. preBuild may work but is not correct because patching is not meant to be done in that phase.

@geistesk @oxzi
Revert "st: copy config file in 'prePatch' instead of 'preBuild'"
50b213a 
Also change the custom config generation to the postPatch phase.
@oxzi oxzi force-pushed the revert-85443-st branch from 50b3849 to 50b213a Compare April 22, 2020 19:19
@oxzi
Copy link
Member Author

oxzi commented Apr 22, 2020

@FRidh: I have updated the commit to add the custom configuration in the postPatch phase. So the patches (inherit patches) are applied first and the configuration is dropped (postPatch) afterwards.
What is your opinion on this?

@Mic92 Mic92 merged commit e0cf3f8 into NixOS:master Apr 29, 2020
@Mic92
Copy link
Member

Mic92 commented Apr 29, 2020

backported:

[detached HEAD 8136104f44b] Revert "st: copy config file in 'prePatch' instead of 'preBuild'"
Author: Alvar geistesk@users.noreply.github.com
Date: Wed Apr 22 13:37:42 2020 +0000
1 file changed, 2 insertions(+), 3 deletions(-)

@oxzi oxzi deleted the revert-85443-st branch April 29, 2020 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@oxzi @FRidh @Mic92 @geistesk