grammar: to statement #1639

kalikiana · 2017-10-25T14:03:34Z

Have you followed the guidelines for contributing?
Have you signed the CLA?
If this is a bugfix. Have you checked that there is a bug report open for the issue you are trying to fix on bug reports?
If this is a new feature. Have you discussed the design on the forum?
Have you successfully run ./runtests.sh static?
Have you successfully run ./runtests.sh unit?

This is the implementation of the "to" statement as discussed in the forum. This PR implements the bold items.
Items 7 and 8 are implemented in the follow-up PR #1800 to allow for quicker review (although I can also add them here if that's preferred, whichever is clearer).

The existing “on :” syntax will be accepted under build-packages as well
The default architecture for packages under “on ” is the one from the build machine
Packages under “on :” are installed only when the build machine has that architecture
A new “to :” statement will be supported in build-packages and stage-packages
The default architecture for packages under “to :” is the target architecture
Packages under “to :” are installed only when the target architecture is the one listed, which includes both a cross-compilation towards that architecture and a native build on that architecture
The two statements may be used together as “on to :” to precisely specify the case of a particular cross-compilation
The default architecture for packages under “on to :” is the target architecture

kyrofa

Nice job @kalikiana, this looks really good. One small typo, and a few test cases are missing. There also seems to be no integration-level tests.

kyrofa · 2017-10-25T19:36:16Z

snapcraft/internal/project_loader/grammar/_to.py

+def _extract_to_clause_selectors(to):
+    """Extract the list of selectors within a to clause.
+
+    :param str to: The 'to <selector' part of the 'to' clause.


Missing a bracket here: 'to <selector>'

kyrofa · 2017-10-25T19:44:34Z

snapcraft/tests/project_loader/grammar/test_to_statement.py

+            ],
+            'target_arch': 'i386',
+            'expected_packages': {'baz'}
+        }),


You're missing a test for ensuring one can still have packages like name:arch without the grammar rewriting the requested arch. Please add a test with this in both the body and an else.

I just realized I didn't actually respond here: I added two new cases, with "arch specified" in the tag, for that. Good call!

kalikiana · 2017-12-11T07:56:59Z

For the record, CI failed with

FAIL: test_failed_registration_already_registered (snapcraft.tests.integration.store.test_store_register.RegisterTestCase) snapcraft.tests.integration.store.test_store_register.RegisterTestCase.test_failed_registration_already_registered
----------------------------------------------------------------------
testtools.testresult.real._StringException: Traceback (most recent call last):
  File "/root/snapcraft/snapcraft/tests/integration/store/test_store_register.py", line 41, in 
test_failed_registration_already_registered
    self.register, 'test-snap-name-already-registered')
  File "/usr/local/lib/python3.5/dist-packages/testtools/testcase.py", line 485, in assertRaises
    self.assertThat(our_callable, matcher)
  File "/usr/local/lib/python3.5/dist-packages/testtools/testcase.py", line 498, in assertThat
    raise mismatch_error
testtools.matchers._impl.MismatchError: <bound method StoreTestCase.register of <snapcraft.tests.integration.store.test_store_register.RegisterTestCase.test_failed_registration_already_registered id=0x7f44d08842e8>> returned None

kyrofa

I've tested this with:

to <host arch>: [hello] and hello was fetched
to <other arch>: [hello]and hello was not fetched
to <other arch>: [hello] with --target-arch=<other arch> and hello:<other arch> was fetched

These seem to be the expected results. I'm going to give this a +1, although I'm not a huge fan of altering other tests' snaps to suit the integration tests here.

kyrofa · 2017-12-11T16:48:02Z

snapcraft/tests/integration/general/test_build_package_grammar.py

@@ -105,3 +108,59 @@ def test_global_build_package_on_other_arch_else(self):
        self.run_snapcraft(['pull'], 'build-package-grammar-global')

        self.assertTrue(self._hello_is_installed())
+
+    @contextlib.contextmanager
+    def modified_yaml(self,


I'd like to get @ElOpio's input on this. I'm not sure how I feel about it. I know we have a lot of test snaps, but warping other snaps to suit this test seems hard to follow. Maybe I'm just used to having YAML to refer to, but I prefer new test snaps.

I don't like either.
I would like a function that makes a yaml, and it takes a string with the contents. This string is optional, if not present it will create the most simple and small valid snapcraft.yaml. This function also takes keyword arguments, for each of the keys of the yaml, so we can modify the basic default yaml.
We have some tests that do something like this, but nothing is consistent. Until we have that, I would prefer to either write the full yaml, or save it to snapcraft/tests/snaps. I don't like this modifier very much.

Thanks for your feedback! It's true we have little consistency, which is why I proposed this.

In an effort to alleviate discussion I went ahead and added a function to construct the YAML from scratch, accepting keyword arguments for toplevel keys with defaults, based on @ElOpio's suggestion above. Although I wasn't entirely sure about the exact details regarding strings.
We should have a hangout to flesh this out.

For the record: @ElOpio and I discussed this and decided to consolidate on extending the fixture to take arguments for parts and build packages like the function proposed here. We don't want a contextmanager, and rather dictionaries than strings. And sharing the fixture across the test suite including unit tests will be easier than making a function available everywhere.

kalikiana · 2017-12-19T15:48:55Z

For the record:

    FAIL: test_grammar 
(snapcraft.tests.integration.general.test_build_snap_grammar.BuildSnapGrammarTestCase) snapcraft.tests.integration.general.test_build_snap_grammar.BuildSnapGrammarTestCase.test_grammar (build-snap-grammar-on)
----------------------------------------------------------------------
testtools.testresult.real._StringException: Traceback (most recent call last):
  File "/root/snapcraft/snapcraft/tests/integration/general/test_build_snap_grammar.py", line 66, in 
setUp
    self.useFixture(fixture_setup.WithoutSnapInstalled('hello'))
  File "/usr/local/lib/python3.5/dist-packages/testtools/testcase.py", line 756, in useFixture
    reraise(*exc_info)
  File "/usr/local/lib/python3.5/dist-packages/testtools/_compat3x.py", line 16, in reraise
    raise exc_obj.with_traceback(exc_tb)
  File "/usr/local/lib/python3.5/dist-packages/testtools/testcase.py", line 731, in useFixture
    fixture.setUp()
  File "/root/snapcraft/snapcraft/tests/fixture_setup.py", line 934, in setUp
    "'sudo snap remove {snap}'.".format(snap=self.snap_name))
AssertionError: This test cannot run if you already have the 'hello' snap installed. Please uninstall it by running 'sudo snap remove hello'.

Not sure how this is happening. Locally it's fine and I didn't modify the test setup...

sergiusens · 2017-12-21T14:39:56Z

For your error the only thing I can think of is that there is some sort of dictionary collision/squatting somewhere along the way

This reverts commit 90f82ad.

please review again

kyrofa · 2018-01-10T19:48:18Z

snapcraft/tests/unit/project_loader/grammar/test_to_statement.py

+    def test_to_statement_grammar(self, platform_machine_mock,
+                                  platform_architecture_mock):
+        platform_machine_mock.return_value = 'x86_64'
+        platform_architecture_mock.return_value = ('64bit', 'ELF')


Why are we setting this, here? to doesn't care about the build host, right? Just the target arch, which we're setting here. Don't we actually want to make sure this runs on other architectures?

kyrofa

Still not a fan of the tests, but if @ElOpio is happy, I'm happy.

kyrofa · 2018-01-10T19:57:34Z

snapcraft/tests/integration/__init__.py

@@ -196,6 +197,25 @@ def copy_project_to_cwd(self, project_dir: str) -> None:
            os.path.join(self.snaps_dir, project_dir), self.path,
            preserve_symlinks=True)

+    def construct_yaml(self, name='test', version='0.1',


This is better than it was before, but I feel like this is introducing yet another inconsistency in how we're doing this. I'd still rather see snaps in integration/snaps/ that we can test standalone as well, like the other grammar tests. However, I've already made that argument, so I won't block on it.

Christian added 3 commits October 24, 2017 17:43

grammar: introduce "to" statement

df3067a

Unit tests for to statement

cfca42c

Default to target arch in to statements

28471aa

kyrofa reviewed Oct 25, 2017

View reviewed changes

sergiusens added this to the 2.36 milestone Oct 26, 2017

Christian added 4 commits October 27, 2017 15:54

Add closing angular bracket in _extract_to_clause_selectors docs

ab1ec61

Add test cases for specified architecture suffix

1fc529e

Add to selector to schema

63feb2e

Add integration tests for to in build packages

3a891bd

sergiusens removed this from the 2.36 milestone Nov 14, 2017

Christian added 3 commits December 8, 2017 10:24

Merge branch 'master' into to_statement

68f8ee2

Integration tests for to with stage packages

6731f50

Fix typo

bd9f96c

kalikiana mentioned this pull request Dec 8, 2017

grammar: on..to statement #1800

Closed

6 tasks

kalikiana mentioned this pull request Dec 11, 2017

Implement "on <arch> to" selectors (statements) in project_loader #1689

Closed

kyrofa previously approved these changes Dec 11, 2017

View reviewed changes

Christian added 3 commits December 13, 2017 13:39

Construct test snaps from scratch

16d2f39

Use SnapcraftYaml with parts and build-packages

90f82ad

Re-instate accidentally removed SnapcraftYaml.path

2f8bd88

Christian and others added 3 commits December 19, 2017 16:49

Merge branch 'master' into to_statement

1fd51ed

Merge branch 'master' into to_statement

348ca0f

Merge branch 'master' into to_statement

c98b0ca

Christian added 2 commits January 9, 2018 11:23

Revert "Use SnapcraftYaml with parts and build-packages"

e190976

This reverts commit 90f82ad.

Merge branch 'master' into to_statement

68a75ad

kyrofa reviewed Jan 10, 2018

View reviewed changes

kyrofa approved these changes Jan 10, 2018

View reviewed changes

sergiusens modified the milestone: 2.39 Jan 10, 2018

sergiusens merged commit f5e82e5 into canonical:master Jan 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

grammar: to statement #1639

grammar: to statement #1639

kalikiana commented Oct 25, 2017 •

edited

kyrofa left a comment •

edited

kyrofa Oct 25, 2017

kyrofa Oct 25, 2017

kalikiana Dec 8, 2017

kalikiana commented Dec 11, 2017 •

edited

kyrofa left a comment •

edited

kyrofa Dec 11, 2017

come-maiz Dec 11, 2017

kalikiana Dec 13, 2017

kalikiana Dec 14, 2017 •

edited

kalikiana commented Dec 19, 2017

sergiusens commented Dec 21, 2017

kyrofa Jan 10, 2018 •

edited

kyrofa left a comment

kyrofa Jan 10, 2018

grammar: to statement #1639

grammar: to statement #1639

Conversation

kalikiana commented Oct 25, 2017 • edited

kyrofa left a comment • edited

Choose a reason for hiding this comment

kyrofa Oct 25, 2017

Choose a reason for hiding this comment

kyrofa Oct 25, 2017

Choose a reason for hiding this comment

kalikiana Dec 8, 2017

Choose a reason for hiding this comment

kalikiana commented Dec 11, 2017 • edited

kyrofa left a comment • edited

Choose a reason for hiding this comment

kyrofa Dec 11, 2017

Choose a reason for hiding this comment

come-maiz Dec 11, 2017

Choose a reason for hiding this comment

kalikiana Dec 13, 2017

Choose a reason for hiding this comment

kalikiana Dec 14, 2017 • edited

Choose a reason for hiding this comment

kalikiana commented Dec 19, 2017

sergiusens commented Dec 21, 2017

kyrofa Jan 10, 2018 • edited

Choose a reason for hiding this comment

kyrofa left a comment

Choose a reason for hiding this comment

kyrofa Jan 10, 2018

Choose a reason for hiding this comment

kalikiana commented Oct 25, 2017 •

edited

kyrofa left a comment •

edited

kalikiana commented Dec 11, 2017 •

edited

kyrofa left a comment •

edited

kalikiana Dec 14, 2017 •

edited

kyrofa Jan 10, 2018 •

edited