Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
go plugin: cross-compilation support #1253
Conversation
sergiusens
requested a review
from
chipaca
Apr 13, 2017
|
I've added someone as a reviewer who will give you some magic lines |
|
At a glance, this is missing If you want to suffer, make that a simple go program that uses a shared C++ library. Mix both of these if you want to lose your mind. The best quick read for this that I know of is still xnox's blog post from a few years ago. Also note the intricacies of this differ between go versions, so test it on all of them. |
|
As a start, try this: package main
/*
#cgo pkg-config: glib-2.0
#cgo LDFLAGS: -lm
#include <math.h>
#include <glib.h>
*/
import "C"
import (
"fmt"
"os/user"
)
func main() {
u, err := user.Current()
if err != nil {
panic(err)
}
fmt.Println("user according to go:", u.Username)
fmt.Println("user according to glib:", C.GoString((*C.char)(C.g_get_user_name())))
fmt.Println("more or less 1:", C.sin(3.14/2))
} |
|
you might need to commit --reauthor your commit with christian@twotoasts.de or sign the CLA ;-) |
|
@mwhudson mind taking a look at this? |
|
It all looks OK to me. |
elopio
requested changes
Apr 19, 2017
@kalikiana, please add a manual test for this, in manual_tests.md.
| + } | ||
| + env['GOARCH'] = go_archs.get(self.project.deb_arch, | ||
| + self.project.deb_arch) | ||
| + env['GOARM'] = '7' |
sergiusens
May 18, 2017
Collaborator
I would add an if self.project.debarch == 'armhf' here, GOARM is probably 8 for arm64 anyways
| + If you want to add other functionalities to this snap, please don't. | ||
| + Make a new one. | ||
| + | ||
| +icon: icon.png |
| @@ -31,6 +33,7 @@ | ||
| from testtools.matchers import MatchesRegex | ||
| from snapcraft.tests import fixture_setup | ||
| +from snapcraft.internal.repo import _deb |
sergiusens
Apr 24, 2017
Collaborator
this gives me a bad vibe, not sure how to solve it yet. Maybe reimplement here, @elopio ?
| @@ -129,6 +132,32 @@ def run_snapcraft_parser(self, arguments): | ||
| raise | ||
| return snapcraft_output | ||
| + @contextmanager | ||
| + def setup_multi_arch_sources(self, arch): |
elopio
Apr 24, 2017
Member
This is nice, but in tests we usually have fixtures instead of context managers. I personally would prefer to follow that practice, but I don't have a very strong opinion against this context manager.
| + | ||
| + arch = 'armhf' | ||
| + with self.setup_multi_arch_sources(arch): | ||
| + self.run_snapcraft('stage', 'go-cgo', '--target={}'.format(arch)) |
| + ('i386', dict(deb_arch='i386', go_arch='386')), | ||
| + ('x86_64', dict(deb_arch='amd64', go_arch='amd64')), | ||
| + ('ppc64le', dict(deb_arch='ppc64el', go_arch='ppc64le')), | ||
| + ] |
elopio
reviewed
Apr 24, 2017
looks very good to me. I'm +1, with the request to move the scenarios before the test, and a comment about using a fixture instead of a context manager.
sergiusens
changed the title from
Cross-compilation support for the Go plugin
to
go plugin: cross-compilation support
Apr 26, 2017
kalikiana
added some commits
Apr 13, 2017
codecov-io
commented
Apr 28, 2017
•
Edited 1 time
-
codecov-io
May 10, 2017
-
May 10, 2017
Codecov Report
@@ Coverage Diff @@
## master #1253 +/- ##
==========================================
- Coverage 95.33% 95.17% -0.16%
==========================================
Files 218 224 +6
Lines 20258 20880 +622
Branches 1612 1672 +60
==========================================
+ Hits 19312 19873 +561
- Misses 672 705 +33
- Partials 274 302 +28
Continue to review full report at Codecov.
|
| + # Add multi-arch suffix for cross-compilation to dev packages | ||
| + for package in part.code.build_packages: | ||
| + if self._project_options.is_cross_compiling: | ||
| + if package.endswith('-dev'): |
sergiusens
Apr 29, 2017
Collaborator
to be future proof and this will happen, we need to check for : to detect a hardcoding of an arch, an all entry or an any; in the case of no : or any I think is when we can append.
cjwatson
Apr 29, 2017
Contributor
I'm pretty sure that the whole approach of appending architecture suffixes is wrong here, not least because apt has a whole load of non-trivial metadata about which packages need which kind of multi-arch treatment that's being ignored. Just checking for -dev suffixes is going to go wrong in both directions.
Here's the relevant spec for the system-level behaviour. To use this correctly, you need to somehow arrange to call apt-get build-dep -a<target-arch> (the behaviour of apt-get build-dep behaves differently in subtle but important ways from apt-get install - for instance the :native suffix isn't defined for ordinary dependencies, only build-dependencies).
My recommendation would be a somewhat different strategy, which is a bit more work but should be a lot more robust: generate a temporary .dsc file with the contents of build-packages in its Build-Depends, and pass that to apt-get build-dep or apt-get build-dep -a<target-arch> depending on whether you're cross-compiling. On xenial you can just pass the .dsc directly to apt-get build-dep, but if you need to support trusty you need to faff about with turning it into a Sources file the same kind of way that sbuild does.
If it were me then I'd do that in a separate PR and rebase this one on top of it later ...
chrmod
referenced this pull request
in chrmod/snap-mosh
Apr 30, 2017
Open
armhf cross compilation on travis #1
kalikiana
added some commits
May 5, 2017
sergiusens
dismissed
elopio’s
stale review
May 11, 2017
needs a new review
sergiusens
added this to the 2.30 milestone
May 12, 2017
kalikiana
added some commits
May 15, 2017
sergiusens
requested changes
May 18, 2017
Looks good, nothing major on my side but I've taken the liberty to request some changes given the fact that a conflict resolution is in order anyways.
Thanks
| @@ -186,20 +188,91 @@ def get_packages_for_source_type(cls, source_type): | ||
| return packages | ||
| @classmethod | ||
| - def install_build_packages(cls, package_names): | ||
| - unique_packages = set(package_names) | ||
| + def _setup_multi_arch(cls, apt_cache, package_name): |
sergiusens
May 18, 2017
Collaborator
Given the complexity (not the code, but the task, it is invasive after all), can we add a docstring?
| + release = platform.linux_distribution()[2] | ||
| + sources = _format_sources_list(None, deb_arch=arch, | ||
| + release=release, foreign=True) | ||
| + with tempfile.NamedTemporaryFile() as sources_arch: |
sergiusens
May 18, 2017
Collaborator
can you rename source_arch to source_arch_file`? makes the code below a faster read
| + return package_name in apt_cache | ||
| + | ||
| + @classmethod | ||
| + def _get_build_deps(cls, package_names, arch): |
| + } | ||
| + env['GOARCH'] = go_archs.get(self.project.deb_arch, | ||
| + self.project.deb_arch) | ||
| + env['GOARM'] = '7' |
sergiusens
May 18, 2017
Collaborator
I would add an if self.project.debarch == 'armhf' here, GOARM is probably 8 for arm64 anyways
| + tempfile.gettempprefix()) | ||
| + if arch: | ||
| + arch = ':{}'.format(arch) | ||
| + prolog = '''NOTE: This is only a simulation! |
sergiusens
May 18, 2017
Collaborator
mind using dedent here? from textwrap import dedent (look at the new-cli branch occurrences for quick inspiration)
| + errors.append(note.format(package, arch)) | ||
| + if not_cached or not_available: | ||
| + self.exception = True | ||
| + details = '''Some packages could not be installed. This may mean that you have |
kalikiana
added some commits
May 19, 2017
|
Seeing some issues First try, go is not installed Second try, first without x-compile Now I can x-compile, but wait... |
| + | ||
| + @classmethod | ||
| + def install_build_packages(cls, package_names, arch): | ||
| + unique_packages = set(cls._get_build_deps(package_names, arch)) |
elopio
May 19, 2017
Member
This is crashing for me:
/home/ubuntu/workspace/canonical/snapcraft/snapcraft/internal/repo/_deb.py(286)install_build_packages()
-> new_packages = []
(Pdb) unique_packages
{'for', 'Keep', 'privileges', 'so', 'package', 'get', 'build', 'in', 'relevance', "don't", "'/tmp/tmpvnu0mvxc.dsc'", 'tree...', 'file', 'the', 'is', 'dependencies', 'depend', 'real', 'on', 'also', 'deactivated,', 'locking', 'execution.', 'Reading', 'information...', 'current', 'needs', 'Building', 'mind', 'using', 'root', 'state', 'to', 'situation!', 'lists...', 'that', 'Note,', 'dependency'}
$ apt-get build-dep -q -s -aarm64 golang-go:native
NOTE: This is only a simulation!
apt-get needs root privileges for real execution.
Keep also in mind that locking is deactivated,
so don't depend on the relevance to the real current situation!
E: No architecture information available for arm64. See apt.conf(5) APT::Architectures for setup
sergiusens
modified the milestones:
2.31,
2.30
May 19, 2017
kalikiana
added some commits
May 23, 2017
| + env['CC'] = '{}-gcc'.format(self.project.arch_triplet) | ||
| + env['CXX'] = '{}-g++'.format(self.project.arch_triplet) | ||
| + env['CGO_ENABLED'] = '1' | ||
| + env['PKG_CONFIG_PATH'] = '/usr/lib/{}/pkgconfig'.format( |
kyrofa
May 29, 2017
•
Edited 1 time
-
kyrofa
May 29, 2017
Member
-
May 29, 2017
This should not be needed. Check the environment bits in project loader for more details.
| + if not packages: | ||
| + packages = ['./{}/...'.format(self._get_local_go_package())] | ||
| + for package in packages: | ||
| + binary = self._gopath_bin + '/' + self._binary_name(package) |
kyrofa
May 29, 2017
•
Edited 1 time
-
kyrofa
May 29, 2017
Member
-
May 29, 2017
In order to maintain consistency with the rest of the codebase, I suggest os.path.join() here.
kyrofa
reviewed
May 29, 2017
Can we split this PR in two, please?
- Go-specific changes that will only work if changing dpkg arch by hand
- The repo changes that add dpkg bits automatically (protected by a prompt)
kalikiana
added some commits
May 29, 2017
sergiusens
modified the milestones:
2.31,
2.30.1
Jun 1, 2017
|
This branch was split into smaller following pieces: |
kalikiana commentedApr 13, 2017
•
Edited 1 time
-
kalikiana
May 5, 2017
To be used with the --target-arch= argument.
Fixes bug 1664857.