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.

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

build: store the patches config in a json file #15395

Merged
merged 1 commit into from Nov 5, 2018

Conversation

Projects
None yet
2 participants
@alexeykuzmin
Contributor

alexeykuzmin commented Oct 25, 2018

Description of Change

$ ./script/apply_all_patches.py patches/common/config.json

Checklist

  • PR description included and stakeholders cc'd
  • npm test passes
  • PR title follows semantic commit guidelines

Release Notes

Notes: no notes

@alexeykuzmin alexeykuzmin requested a review from nornagon Oct 25, 2018

@alexeykuzmin alexeykuzmin requested a review from electron/reviewers as a code owner Oct 25, 2018

@nornagon

This comment has been minimized.

Contributor

nornagon commented Oct 25, 2018

Would calling git-import-patches twice work for this?

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Oct 25, 2018

Would calling git-import-patches twice work for this?

You mean something like this?

cd src
git import-patches .../my-patches/chromium
cd electron
git import-patches .../my-patches/electron
if [ "$CONDITION" != "" ]; then
  git import-patches .../my-other-patches/electron
fi

it would be one weird looking script.

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Oct 25, 2018

My goal is to use apply_all_patches.py in a DEPS file, just like Electron does that.

@nornagon

This comment has been minimized.

Contributor

nornagon commented Oct 25, 2018

Would calling git-import-patches twice work for this?

You mean something like this?

cd src
git import-patches .../my-patches/chromium
cd electron
git import-patches .../my-patches/electron
if [ "$CONDITION" != "" ]; then
  git import-patches .../my-other-patches/electron
fi

it would be one weird looking script.

  1. you can use git -C src import-patches if electron/script is in your PATH.
  2. you'd need the if anyway even with this change
@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Oct 26, 2018

you can use git -C src import-patches if electron/script is in your PATH.

I don't run it from a command line, it is not supposed to be run by humans at all.
A script that would add some folder to the PATH and then running git commands, would look at least questionable.

@alexeykuzmin alexeykuzmin added the wip label Oct 26, 2018

@alexeykuzmin alexeykuzmin changed the title from build: add optional "--from-to" arg to the apply_all_patches.py to build: move some common parts to a new "apply_patches.py" script Oct 26, 2018

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Oct 26, 2018

@nornagon Ok, another iteration.
Some shared logic moved to a new script, which can be both imported and used from a command-line.

@alexeykuzmin alexeykuzmin removed the wip label Oct 26, 2018

@alexeykuzmin alexeykuzmin changed the title from build: move some common parts to a new "apply_patches.py" script to [wip] build: move some common parts to a new "apply_patches.py" script Oct 31, 2018

@alexeykuzmin alexeykuzmin changed the title from [wip] build: move some common parts to a new "apply_patches.py" script to build: move some common parts to a new "apply_patches.py" script Oct 31, 2018

@nornagon

this still irks me. apply_all_patches.py was supposed to be a slightly glorified for loop, and could have been written in bash. i don't love adding a little syntax to it (with the :). i'd rather simplify the scripts so that it's trivial for you to write your own for loop.

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Nov 2, 2018

@nornagon

apply_all_patches.py was supposed to be a slightly glorified for loop, and could have been written in bash. i don't love adding a little syntax to it (with the :).

apply_all_patches.py still does exactly the same, just the for loop has been moved to a new file.

i'd rather simplify the scripts so that it's trivial for you to write your own for loop.

I basically can't import anything from the script/lib/ if code I write is not in the electron repo.

@nornagon

This comment has been minimized.

Contributor

nornagon commented Nov 2, 2018

my concern isn't about changing the behaviour of apply_all_patches.py, it's with complicating the logic.

how about this:

import os
from subprocess import check_call

patches = {
  'patches/foo': 'src/foo',
  'patches/bar': 'src/bar',
}

for dir,src in patches.iteritems():
  check_call(['git', '-C', src, 'import-patches', dir],
      env={'PATH': ['../electron/script'] + os.environ['PATH']})

Another reasonable approach would be to move the list of patch dirs to a patches/config.json or something, and pass the path to that file to apply_all_patches.py instead of hardcoding it. Then apply_all_patches.py could become simply:

#!/usr/bin/env python

import json
import sys

from lib import git
from lib.patches import patch_from_dir


def apply_patches(dirs):
  for patch_dir, repo in dirs.iteritems():
    git.am(repo=repo, patch_data=patch_from_dir(patch_dir),
      committer_name="Electron Scripts", committer_email="scripts@electron")


def main():
  with open(sys.argv[1]) as config_json:
    apply_patches(json.load(config_json))


if __name__ == '__main__':
  main()
@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Nov 5, 2018

Another reasonable approach would be to move the list of patch dirs to a patches/config.json or something

@nornagon
Created a JSON config for the patches.

@alexeykuzmin alexeykuzmin changed the title from build: move some common parts to a new "apply_patches.py" script to build: store the patches config in a json file Nov 5, 2018

@alexeykuzmin alexeykuzmin merged commit 32ea2b6 into master Nov 5, 2018

21 checks passed

Absolute Zero
Semantic Pull Request ready to be squashed
Details
WIP Legacy commit status override — see details
Details
appveyor: win-ia32-debug AppVeyor build succeeded
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-debug AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
ci/circleci: linux-arm-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-checkout Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing-tests Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing-tests Your tests passed on CircleCI!
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

release-clerk bot commented Nov 5, 2018

No Release Notes

@alexeykuzmin alexeykuzmin deleted the configurable-patches branch Nov 5, 2018

@trop

This comment has been minimized.

Contributor

trop bot commented Nov 5, 2018

I have automatically backported this PR to "4-0-x", please check out #15572

@trop trop bot added in-flight/4-0-x and removed target/4-0-x labels Nov 5, 2018

@trop trop bot added merged/4-0-x and removed in-flight/4-0-x labels Nov 5, 2018

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