repo: setup a foreign arch and sources

[kalikiana]

Automatically add the target architecture via dpkg and setup apt sources.list for multi-arch if needed.

• Print host setup instructions for foreign architectures and sources list.
• Distinguish invalid architectures.

Fixes: bug 1660903

Follow-up on Figure out multi-arch build dependencies

[elopio]

hum, we are hardcoding 'utf-8' everywhere. This seems better.

[kyrofa]

Agreed.

[elopio]

Why not mv, so the temp file is not left hanging there?

[kalikiana]

The file is deleted at the end of the with statement.

[elopio]

I reviewed the last commit, and I like it. Do you think this is something Colin should review?

And maybe a manual test?

[kyrofa]

It's not completely clear from the function name or the docstring what the return value indicates.

[kyrofa]

A few suggestions.

[kyrofa]

This is getting complex, and looks error-prone. I can imagine myself asking "what's the second arg... version or arch?" and "the arch doesn't include the colon, right?"

I also see us using name + arch a lot below. I suggest making this a super simple class with a few helper methods to do that type of thing for us, and return an instance of that class here.

[kalikiana]

I dropped those changes from this PR, after moving to using the error message to render instructions needed to setup the host.

[kyrofa]

I know I've said this before, but I'll add my voice here again: I personally don't like this. It means running the wrong snapcraft.yaml on my host will garbage up my arch list and sources. The only place this makes sense is when we're building in containers by default, but we're not there yet. Is there any way we can just warn if we're not building in a container?

[kalikiana]

Warn and then what? The build will fail.

[kyrofa]

Yes, that's exactly what I want. Warn telling me what I need to do if I actually want to build this snap (e.g. this command). Ahem I suppose that would be an error, not a warning.

[kalikiana]

Ah, yeah. I see what you're saying now. We could basically generate the necessary commands and leave it up to you to decide which way to go - to my mind, that's the crucial bit, make it as discoverable as possible what's needed to continue, since it's just tedious to find this out for yourself if you've never done it.

How about something like The host needs to be setup for the target architecture. You can use cleanbuild or set SNAPCRAFT_CONTAINER_BUILDS=1 to build in a container that's automatically set up for cross-compiling. To setup the local machine, run the following commands: [...].
Long term with #1458 we could also consider a prompt like Building in a container is recommended for cross-compiling [...] [y/n]

[kyrofa]

Yeah that's a great error, +1.

[kyrofa]

The PR description doesn't indicate that further work needs to be done. The FIXME isn't particularly helpful, either. What happens if a release contains this before it's fixed? Should we raise an NotImplementedError or something instead?

[kalikiana]

Good catch, I didn't mean to leave that there. It was supposed to be a reminder for myself as I had to re-think the code after merging, but I apparently forgot. Will fix it.

[kyrofa]

I'm curious why we're replacing here as opposed to making it a templated param? We should probably be catching KeyErrors here, by the way, and turning them into warnings since this can come from custom plugins as well.

Assuming we went this route for a good reason, we need to make sure it's robust to sources like deb http://debian.mycompany.com/[...].

[kalikiana]

I sorta considered this internal API used only but Snapcraft itself. But actually you're right, this would benefit from some error handling. The replace was meant to avoid having [arch= in there when it's not needed, but I should use an $arch variable for that.

[kyrofa]

Or just use a $deb variable for the whole thing, yeah, something like that.

[kyrofa]

I fell prey to this as well. This error message is a little backward: a KeyError is raised if the template is not satisfied, not if a substitution was requested but not performed. Code will be more clear:

>>> import string
>>> t = '$food is delicious'
>>> string.Template(t).substitute(food='sushi')
'sushi is delicious'
>>> string.Template(t).substitute(food='sushi', bar='baz')
'sushi is delicious'
>>> string.Template(t).substitute(bar='baz')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.5/string.py", line 129, in substitute
    return self.pattern.sub(convert, self.template)
  File "/usr/lib/python3.5/string.py", line 119, in convert
    val = mapping[named]
KeyError: 'food'

So the error should probably be something more along the lines of "Cannot complete substitution in sources list: unable to define $foo".

[kyrofa]

There doesn't seem to be a way to warn in the reverse situation... hmm...

[kalikiana]

You made me wonder what's actually an error here: if you're using a varaible ${foo} that we don't know how to fill in, that seems wrong - but using all the variables isn't necessary.

So, I updated the error based on your wording suggestion and added two test cases for the above mentioned cases, one of them failing, the other one passing. It seems to make sense this way... but let me know what you think.

[kyrofa]

This looks like it's building on what used to be in errors.PackageNotFoundError. What is the rational for essentially redefining it here as opposed to moving this logic back into it?

[kalikiana]

What do you mean by redefining?

My idea was to move the logic into the error because it needs to know

• if the arch is valid
• if the arch was registered
• if there's sources for the arch
  _mark_install doesn't need to make a distinction if we always want it to fail - previously I had the code in there because it would have had to continue iterating through the packages after automatically setting up the host system.

Also, we might be able to re-use the error with all its details. There's a case of except PackageNotFoundError as e: raise BuildPackageNotFoundError and another instance of raising PackageNotFoundError. Maybe this could be a follow-up.

Maybe having helper methods like has_arch still makes sense when I'm adding the container side of things, but aside from that I feel it would become less readable to do that in the same method as figuring out an error message.

[kalikiana]

Ha, after typing all this, and reading your question once more, I realize you're probably referring to the fact that _deb.PackageNotFoundError is a subclass.
I did that because the logic is completely specific to Debian-based distros and the is this a deb distro-check is implied by being inside _deb as well. Presumably once there's a Fedora/dnf backend it will have completely different code (or none at all) depending on how architectures are handled there.

[kyrofa]

Ah, good call, that makes sense indeed.

[kyrofa]

Looks good, one nit: the format for a variable doesn't REQUIRE braces, it simply supports them. I wonder if simply using the variable name here would be better instead of assuming a specific format.

[kalikiana]

I was kinda going by the fact that we use braces in our templates... but I thought of another option, I'm checking now which one was actually used to refer to it in the error message.

[sergiusens]

stale

[kalikiana]

Why did you close this? It's not stale: the last commit, addressing the test failure, is from Sunday.

[kalikiana]

TypeError: <lambda>() got an unexpected keyword argument 'stdout'