Merge ~tintou/git-build-recipe:master into git-build-recipe:master
- Git
- lp:~tintou/git-build-recipe
- master
- Merge into master
Status: | Needs review |
---|---|
Proposed branch: | ~tintou/git-build-recipe:master |
Merge into: | git-build-recipe:master |
Diff against target: |
230 lines (+92/-25) 2 files modified
gitbuildrecipe/recipe.py (+55/-19) gitbuildrecipe/tests/test_recipe.py (+37/-6) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Colin Watson (community) | Needs Fixing | ||
Review via email: mp+351699@code.launchpad.net |
Commit message
Handle submodules
It is possible to fetch the submodules before building the package by specifying submodule-strategy {normal, recursive} in the header of the recipe
The default behavior is not changed
Description of the change
I had to refactor the handling of the `deb-version` optional variable as there are now two optional variables.
by default, there is no additional command run, if the submodule strategy is changed then the repository is synced and updated (the behavior is inspired from what is described in https:/
I haven't tested it on a real repository though.
Colin Watson (cjwatson) wrote : | # |
Hm. There are going to be some more serious problems too.
URL filtering
-------------
In general, when we have user-supplied URLs, we normally filter them so that people can't e.g. pass in file:/// URLs and use those to construct some kind of exploit against the local system. For URLs in the recipe itself, we handle those by parsing the recipe in Launchpad and only allowing references to repositories hosted on Launchpad. But for submodules, that typically doesn't work; we'll need to expect that repositories have references to external URLs, and it's hard to filter those in advance (especially for the recursive strategy).
Now, it's possible we can get away without caring about this, since the builders on which we run git-build-recipe are throwaway VMs that don't have any secrets. But it needs to be documented for the benefit of other users of git-build-recipe, and it would be good if you could do some research to find if there's any reasonable way to filter submodule URLs before cloning them.
Recipe parsing in Launchpad
-------
Launchpad currently takes the shortcut of parsing the supplied recipe using bzr-builder, and then mangling it slightly to account for the minor differences in git-build-recipe's format. We'll need to check whether that process needs any changes to account for this extension to the format. I think we ought to check that before landing this, because perhaps there will turn out to be some way to extend the format that's more compatible than others.
External access from Launchpad builders
-------
The bad news is that this is the hard bit. The good news is that it doesn't need to block landing this code; it may well make the feature less useful until something's done about it, but the fixes for this won't be in git-build-recipe.
In general, Launchpad builders don't have access to the external internet. So this will for submodules that are hosted on Launchpad (using lp: or https:/
We'll need to have a policy discussion about whether we want to allow these on Launchpad. I'm leaning towards yes, because git submodules are pinned by their commit ID which is pretty good protection against the usual problems with external access for things like build reproducibility, but we still need to have the discussion in the Launchpad team and reach an agreement.
After that, there would be technical work to be done to implement this. We could reuse the proxy that we use to allow snap builders to have external internet access, perhaps dispatching it only to recipes that have a submodule-strategy. That would require changes in lib/lp/
Sergey Ponomarev (stokito) wrote : | # |
Hello, is any progress on this? We need it and will be happy if you finally fix that.
Thank in advance
Dimitri John Ledkov (xnox) wrote : | # |
About URL filtering
gitconfig allows to globally rewrite urls/protocols, which is under the control of the system/user, and not of the repository being cloned.
such that one could rewrite them to invalid ones, or force all of them to go through a custom protocol which then implements the filtering.
But that would require careful engineering & security penetration testing.
Unmerged commits
- b1f7355... by Corentin Noël
-
Handle submodules
It is possible to fetch the submodules before building the package by specifying submodule-strategy {normal, recursive} in the header of the recipe
The default behavior is not changed
Preview Diff
1 | diff --git a/gitbuildrecipe/recipe.py b/gitbuildrecipe/recipe.py | |||
2 | index d1700f2..3f012ff 100644 | |||
3 | --- a/gitbuildrecipe/recipe.py | |||
4 | +++ b/gitbuildrecipe/recipe.py | |||
5 | @@ -281,6 +281,11 @@ class CheckoutFailed(GitCommandFailed): | |||
6 | 281 | _cmd = "checkout" | 281 | _cmd = "checkout" |
7 | 282 | 282 | ||
8 | 283 | 283 | ||
9 | 284 | class SubmoduleFailed(GitCommandFailed): | ||
10 | 285 | |||
11 | 286 | _cmd = "submodule" | ||
12 | 287 | |||
13 | 288 | |||
14 | 284 | class MergeFailed(GitCommandFailed): | 289 | class MergeFailed(GitCommandFailed): |
15 | 285 | 290 | ||
16 | 286 | _cmd = "merge" | 291 | _cmd = "merge" |
17 | @@ -320,6 +325,20 @@ def pull_or_clone(base_branch, target_path): | |||
18 | 320 | base_branch.git_call("checkout", base_branch.commit) | 325 | base_branch.git_call("checkout", base_branch.commit) |
19 | 321 | except subprocess.CalledProcessError as e: | 326 | except subprocess.CalledProcessError as e: |
20 | 322 | raise CheckoutFailed(e.output) | 327 | raise CheckoutFailed(e.output) |
21 | 328 | if base_branch.submodule_strategy is not None: | ||
22 | 329 | if base_branch.submodule_strategy == "recursive": | ||
23 | 330 | try: | ||
24 | 331 | base_branch.git_call("submodule", "sync", "--recursive") | ||
25 | 332 | base_branch.git_call("submodule", "update", "--init", | ||
26 | 333 | "--recursive") | ||
27 | 334 | except subprocess.CalledProcessError as e: | ||
28 | 335 | raise SubmoduleFailed(e.output) | ||
29 | 336 | else: | ||
30 | 337 | try: | ||
31 | 338 | base_branch.git_call("submodule", "sync") | ||
32 | 339 | base_branch.git_call("submodule", "update", "--init") | ||
33 | 340 | except subprocess.CalledProcessError as e: | ||
34 | 341 | raise SubmoduleFailed(e.output) | ||
35 | 323 | 342 | ||
36 | 324 | 343 | ||
37 | 325 | def fetch_branches(child_branch): | 344 | def fetch_branches(child_branch): |
38 | @@ -608,7 +627,7 @@ class RecipeBranch: | |||
39 | 608 | `self.url`. | 627 | `self.url`. |
40 | 609 | """ | 628 | """ |
41 | 610 | 629 | ||
43 | 611 | def __init__(self, name, url, revspec=None): | 630 | def __init__(self, name, url, submodule_strategy=None, revspec=None): |
44 | 612 | """Create a `RecipeBranch`. | 631 | """Create a `RecipeBranch`. |
45 | 613 | 632 | ||
46 | 614 | :param name: The name for the branch, or None if it is the root. | 633 | :param name: The name for the branch, or None if it is the root. |
47 | @@ -618,6 +637,7 @@ class RecipeBranch: | |||
48 | 618 | """ | 637 | """ |
49 | 619 | self.name = name | 638 | self.name = name |
50 | 620 | self.url = url | 639 | self.url = url |
51 | 640 | self.submodule_strategy = submodule_strategy | ||
52 | 621 | self.revspec = revspec | 641 | self.revspec = revspec |
53 | 622 | self.child_branches = [] | 642 | self.child_branches = [] |
54 | 623 | self.commit = None | 643 | self.commit = None |
55 | @@ -796,13 +816,14 @@ class RecipeBranch: | |||
56 | 796 | class BaseRecipeBranch(RecipeBranch): | 816 | class BaseRecipeBranch(RecipeBranch): |
57 | 797 | """The `RecipeBranch` that is at the root of a recipe.""" | 817 | """The `RecipeBranch` that is at the root of a recipe.""" |
58 | 798 | 818 | ||
60 | 799 | def __init__(self, url, deb_version, format_version, revspec=None): | 819 | def __init__(self, url, deb_version, format_version, |
61 | 820 | submodule_strategy=None, revspec=None): | ||
62 | 800 | """Create a `BaseRecipeBranch`. | 821 | """Create a `BaseRecipeBranch`. |
63 | 801 | 822 | ||
64 | 802 | :param deb_version: The template to use for the version number. | 823 | :param deb_version: The template to use for the version number. |
65 | 803 | Should be None for anything except the root branch. | 824 | Should be None for anything except the root branch. |
66 | 804 | """ | 825 | """ |
68 | 805 | super().__init__(None, url, revspec=revspec) | 826 | super().__init__(None, url, submodule_strategy, revspec=revspec) |
69 | 806 | self.deb_version = deb_version | 827 | self.deb_version = deb_version |
70 | 807 | self.format_version = format_version | 828 | self.format_version = format_version |
71 | 808 | if not urlparse(url).scheme: | 829 | if not urlparse(url).scheme: |
72 | @@ -910,8 +931,9 @@ class RecipeParser: | |||
73 | 910 | self.current_indent_level = 0 | 931 | self.current_indent_level = 0 |
74 | 911 | self.seen_nicks = set() | 932 | self.seen_nicks = set() |
75 | 912 | self.seen_paths = {".": 1} | 933 | self.seen_paths = {".": 1} |
77 | 913 | version, deb_version = self.parse_header() | 934 | version, deb_version, submodule_strategy = self.parse_header() |
78 | 914 | self.version = version | 935 | self.version = version |
79 | 936 | self.submodule_strategy = submodule_strategy | ||
80 | 915 | last_instruction = None | 937 | last_instruction = None |
81 | 916 | active_branches = [] | 938 | active_branches = [] |
82 | 917 | last_branch = None | 939 | last_branch = None |
83 | @@ -940,7 +962,8 @@ class RecipeParser: | |||
84 | 940 | revspec = self.parse_optional_revspec() | 962 | revspec = self.parse_optional_revspec() |
85 | 941 | self.new_line() | 963 | self.new_line() |
86 | 942 | last_branch = BaseRecipeBranch( | 964 | last_branch = BaseRecipeBranch( |
88 | 943 | url, deb_version, self.version, revspec=revspec) | 965 | url, deb_version, self.version, self.submodule_strategy, |
89 | 966 | revspec=revspec) | ||
90 | 944 | active_branches = [last_branch] | 967 | active_branches = [last_branch] |
91 | 945 | last_instruction = "" | 968 | last_instruction = "" |
92 | 946 | else: | 969 | else: |
93 | @@ -968,7 +991,8 @@ class RecipeParser: | |||
94 | 968 | else: | 991 | else: |
95 | 969 | revspec = self.parse_optional_revspec() | 992 | revspec = self.parse_optional_revspec() |
96 | 970 | self.new_line(instruction) | 993 | self.new_line(instruction) |
98 | 971 | last_branch = RecipeBranch(branch_id, url, revspec=revspec) | 994 | last_branch = RecipeBranch(branch_id, url, |
99 | 995 | self.submodule_strategy, revspec=revspec) | ||
100 | 972 | if instruction == NEST_INSTRUCTION: | 996 | if instruction == NEST_INSTRUCTION: |
101 | 973 | active_branches[-1].nest_branch(location, last_branch) | 997 | active_branches[-1].nest_branch(location, last_branch) |
102 | 974 | elif instruction == MERGE_INSTRUCTION: | 998 | elif instruction == MERGE_INSTRUCTION: |
103 | @@ -994,9 +1018,14 @@ class RecipeParser: | |||
104 | 994 | if version > self.NEWEST_VERSION: | 1018 | if version > self.NEWEST_VERSION: |
105 | 995 | self.throw_parse_error("Unknown format: '%s'" % str(version)) | 1019 | self.throw_parse_error("Unknown format: '%s'" % str(version)) |
106 | 996 | self.take_chars(len(version_str)) | 1020 | self.take_chars(len(version_str)) |
108 | 997 | deb_version = self.parse_optional_deb_version() | 1021 | options = self.parse_optional_values() |
109 | 1022 | deb_version = options.get('deb-version', None) | ||
110 | 1023 | submodule_strategy = options.get('submodule-strategy', None) | ||
111 | 1024 | if submodule_strategy not in (None, 'normal', 'recursive'): | ||
112 | 1025 | self.throw_parse_error("Unknown submodule strategy: '%s'" % | ||
113 | 1026 | str(submodule_strategy)) | ||
114 | 998 | self.new_line() | 1027 | self.new_line() |
116 | 999 | return version, deb_version | 1028 | return version, deb_version, submodule_strategy |
117 | 1000 | 1029 | ||
118 | 1001 | def parse_instruction(self, permitted_instructions=None): | 1030 | def parse_instruction(self, permitted_instructions=None): |
119 | 1002 | if self.version < 0.2: | 1031 | if self.version < 0.2: |
120 | @@ -1079,17 +1108,24 @@ class RecipeParser: | |||
121 | 1079 | self.parse_whitespace("the revspec") | 1108 | self.parse_whitespace("the revspec") |
122 | 1080 | return self.take_to_whitespace("the revspec") | 1109 | return self.take_to_whitespace("the revspec") |
123 | 1081 | 1110 | ||
135 | 1082 | def parse_optional_deb_version(self): | 1111 | def parse_optional_values(self): |
136 | 1083 | self.parse_whitespace("'deb-version'", require=False) | 1112 | options = dict() |
137 | 1084 | actual = self.peek_to_whitespace() | 1113 | expected_options = ("deb-version", "submodule-strategy") |
138 | 1085 | if actual is None: | 1114 | for i in range(len(expected_options)): |
139 | 1086 | # End of line, no version | 1115 | self.parse_whitespace("next option", require=False) |
140 | 1087 | return None | 1116 | actual = self.peek_to_whitespace() |
141 | 1088 | if actual != "deb-version": | 1117 | if actual is None: |
142 | 1089 | self.throw_expecting_error("deb-version", actual) | 1118 | # End of line |
143 | 1090 | self.take_chars(len("deb-version")) | 1119 | continue; |
144 | 1091 | self.parse_whitespace("a value for 'deb-version'") | 1120 | else: |
145 | 1092 | return self.take_to_whitespace("a value for 'deb-version'") | 1121 | if actual not in expected_options: |
146 | 1122 | self.throw_expecting_error("' or '".join(expected_options), | ||
147 | 1123 | actual) | ||
148 | 1124 | self.take_chars(len(actual)) | ||
149 | 1125 | self.parse_whitespace("a value for '%s'" % actual) | ||
150 | 1126 | options[actual] = self.take_to_whitespace( | ||
151 | 1127 | "a value for '%s'" % actual) | ||
152 | 1128 | return options | ||
153 | 1093 | 1129 | ||
154 | 1094 | def parse_optional_revspec(self): | 1130 | def parse_optional_revspec(self): |
155 | 1095 | self.parse_whitespace(None, require=False) | 1131 | self.parse_whitespace(None, require=False) |
156 | diff --git a/gitbuildrecipe/tests/test_recipe.py b/gitbuildrecipe/tests/test_recipe.py | |||
157 | index ed7634d..fdee41e 100644 | |||
158 | --- a/gitbuildrecipe/tests/test_recipe.py | |||
159 | +++ b/gitbuildrecipe/tests/test_recipe.py | |||
160 | @@ -95,6 +95,17 @@ class RecipeParserTests(GitTestCase): | |||
161 | 95 | branch = "http://foo.org/\n" | 95 | branch = "http://foo.org/\n" |
162 | 96 | self.get_recipe(header + branch) | 96 | self.get_recipe(header + branch) |
163 | 97 | 97 | ||
164 | 98 | def test_parses_reversed_strategy_deb_version(self): | ||
165 | 99 | header = ("# git-build-recipe format 0.4 submodule-strategy normal " | ||
166 | 100 | "deb-version " + self.deb_version + "\n") | ||
167 | 101 | branch = "http://foo.org/\n" | ||
168 | 102 | self.get_recipe(header + branch) | ||
169 | 103 | |||
170 | 104 | def test_parses_missing_deb_version_with_strategy(self): | ||
171 | 105 | header = "# git-build-recipe format 0.4 submodule-strategy normal\n" | ||
172 | 106 | branch = "http://foo.org/\n" | ||
173 | 107 | self.get_recipe(header + branch) | ||
174 | 108 | |||
175 | 98 | def tests_rejects_non_comment_to_start(self): | 109 | def tests_rejects_non_comment_to_start(self): |
176 | 99 | self.assertParseError(1, 1, "Expecting '#', got 'g'", | 110 | self.assertParseError(1, 1, "Expecting '#', got 'g'", |
177 | 100 | self.get_recipe, "git-build-recipe") | 111 | self.get_recipe, "git-build-recipe") |
178 | @@ -128,9 +139,9 @@ class RecipeParserTests(GitTestCase): | |||
179 | 128 | self.get_recipe, | 139 | self.get_recipe, |
180 | 129 | "# git-build-recipe format 10000 deb-version 1\n") | 140 | "# git-build-recipe format 10000 deb-version 1\n") |
181 | 130 | 141 | ||
185 | 131 | def test_rejects_invalid_deb_version_marker(self): | 142 | def test_rejects_invalid_parameter(self): |
186 | 132 | self.assertParseError(1, 31, "Expecting 'deb-version', " | 143 | self.assertParseError(1, 31, "Expecting 'deb-version' or " |
187 | 133 | "got 'deb'", self.get_recipe, | 144 | "'submodule-strategy', got 'deb'", self.get_recipe, |
188 | 134 | "# git-build-recipe format 0.1 deb") | 145 | "# git-build-recipe format 0.1 deb") |
189 | 135 | 146 | ||
190 | 136 | def tests_rejects_no_deb_version_value(self): | 147 | def tests_rejects_no_deb_version_value(self): |
191 | @@ -138,9 +149,19 @@ class RecipeParserTests(GitTestCase): | |||
192 | 138 | "a value for 'deb-version'", self.get_recipe, | 149 | "a value for 'deb-version'", self.get_recipe, |
193 | 139 | "# git-build-recipe format 0.1 deb-version") | 150 | "# git-build-recipe format 0.1 deb-version") |
194 | 140 | 151 | ||
198 | 141 | def tests_rejects_extra_text_after_deb_version(self): | 152 | def test_unknown_submodule_strategy(self): |
199 | 142 | self.assertParseError(1, 45, "Expecting the end of the line, " | 153 | self.assertParseError(1, 53, "Unknown submodule strategy: 'foo'", |
200 | 143 | "got 'foo'", self.get_recipe, | 154 | self.get_recipe, |
201 | 155 | "# git-build-recipe format 0.4 submodule-strategy foo\n") | ||
202 | 156 | |||
203 | 157 | def tests_rejects_no_submodule_strategy_value(self): | ||
204 | 158 | self.assertParseError(1, 49, "End of line while looking for " | ||
205 | 159 | "a value for 'submodule-strategy'", self.get_recipe, | ||
206 | 160 | "# git-build-recipe format 0.4 submodule-strategy") | ||
207 | 161 | |||
208 | 162 | def tests_rejects_extra_parameter(self): | ||
209 | 163 | self.assertParseError(1, 45, "Expecting 'deb-version' or " | ||
210 | 164 | "'submodule-strategy', got 'foo'", self.get_recipe, | ||
211 | 144 | "# git-build-recipe format 0.1 deb-version 1 foo") | 165 | "# git-build-recipe format 0.1 deb-version 1 foo") |
212 | 145 | 166 | ||
213 | 146 | def tests_rejects_indented_base_branch(self): | 167 | def tests_rejects_indented_base_branch(self): |
214 | @@ -539,6 +560,16 @@ class BuildTreeTests(GitTestCase): | |||
215 | 539 | self.assertEqual(commit, target.rev_parse("HEAD")) | 560 | self.assertEqual(commit, target.rev_parse("HEAD")) |
216 | 540 | self.assertEqual(commit, base_branch.commit) | 561 | self.assertEqual(commit, base_branch.commit) |
217 | 541 | 562 | ||
218 | 563 | def test_build_tree_single_branch_recursive(self): | ||
219 | 564 | source = GitRepository("source") | ||
220 | 565 | commit = source.commit("one") | ||
221 | 566 | base_branch = BaseRecipeBranch("source", "1", 0.2, "recursive") | ||
222 | 567 | build_tree(base_branch, "target") | ||
223 | 568 | self.assertThat("target", PathExists()) | ||
224 | 569 | target = GitRepository("target", allow_create=False) | ||
225 | 570 | self.assertEqual(commit, target.rev_parse("HEAD")) | ||
226 | 571 | self.assertEqual(commit, base_branch.commit) | ||
227 | 572 | |||
228 | 542 | def test_build_tree_single_branch_dir_not_branch(self): | 573 | def test_build_tree_single_branch_dir_not_branch(self): |
229 | 543 | source = GitRepository("source") | 574 | source = GitRepository("source") |
230 | 544 | source.commit("one") | 575 | source.commit("one") |
Most of these are nits, but I'd really prefer to have end-to-end functional tests before merging this (although I've tested it locally and it does seem to work fine).