options: use actual userspace architecture for building in 32bit #1060

3v1n0 · Jan 20, 2017

It might happen in 32bit containers running on 64bit hosts, that
they are detected as 64bit systems since platform's machine()
returns the kernel's architecture, not the userspace one.

Using gcc -v Target:'s value instead

LP: #1641123

kyrofa · Jan 20, 2017

Looks like some static test failures here.

codecov-io · Jan 23, 2017

Current coverage is 96.34% (diff: 100%)

Merging #1060 into master will increase coverage by <.01%

@@             master      #1060   diff @@
==========================================
  Files           194        194          
  Lines         17617      17658    +41   
  Methods           0          0          
  Messages          0          0          
  Branches       1355       1359     +4   
==========================================
+ Hits          16971      17012    +41   
  Misses          440        440          
  Partials        206        206

Powered by Codecov. Last update ff28c71...930ced1

adconrad · Jan 23, 2017

You can (and should) skip the ^Target regex entirely here by using 'gcc -dumpmachine' instead of parsing the output of 'gcc -v'

It's worth noting, however, that triplets will differ between distros. Not everyone uses the upstream defaults. Some may say i386-linux-gnu instead of i686, some may include the vendor tag (ie: i686-redhat-linux-gnu), some alter the machine instead of the ABI (armhl-linux-gnu instead of arm-linux-gnueabihf).

That's not to say this approach is wrong, but there are lots of small corner cases you'll have to cater to in a switch to get it right for all your target platforms. Thankfully, logging in to a bunch of different distros and testing this out probably isn't super hard, but it's certainly annoying.

3v1n0 · Jan 23, 2017

@adconrad mh, I've actually followed what it was written in the related bug as it's what @sergiusens also suggested; my first implementation was actually using a different approach (see commit) which was using python's platform architecture (which checks for the arch of a binary file - python by default), and that's probably more universal than this then.

I can go back to that way instead... To me checking the python binary type that has been launched seems pretty sane, but.... Indeed there could be still many corner cases.

sergiusens · Jan 24, 2017

sergiusens requested changes Jan 24, 2017

View changes

Thanks for this, looks good except for one thing in the nodejs plugin, hope you don't mind doing what I suggest.
Thanks

hmm, can't you pass project_options.deb_arch and change the map to use deb_arch instead?

sergiusens · Jan 25, 2017

sergiusens reviewed Jan 25, 2017

View changes

hey, a couple of comments, almost there

you should pass self.project.debarch

Ah, ok... I thought you meant to pass the whole project in the previous req...

this should recieve machine

sergiusens · Jan 25, 2017

sergiusens approved these changes Jan 25, 2017

View changes

Great! Sorry for the back and forth, you were right!

adconrad · Jan 25, 2017

I think the "just ask python" method seems cleaner. Yes, it won't work in cross-build cases, but those are explicitly not catered for here and, really, the only sane way to cross is to explicitly request an arch via a flag, not hope you can guess implicitly.

In the dpkg world, dpkg-dev will fall back to 'gcc -dumpmachine' (which might be where people got the idea from) if there's no dpkg binary around to talk to, but that's a worst case bootstrapping scenario. Usually, we have an actual dpkg binary to ask about itself, and we trust the answer. The analog here of trusting python seems like the correct one, and perhaps making sure you have the hooks in place for a future explicit passing of arch for crossing.

adconrad · Jan 25, 2017

(And sorry if any of my comments along the way ended up adding to the confusion instead of helping resolve it)

3v1n0 · Jan 25, 2017

@adconrad no worries, I'm happy we ended up with this decision too...

making sure you have the hooks in place for a future explicit passing of arch for crossing.

Well the --target-arch is already there and it's exactly for that reason I believe.

3v1n0 changed the title from options: if host is 32bit use the relative backward compatibility architecture to options: use gcc to get the actual host/target architecture Jan 23, 2017

3v1n0 changed the title from options: use gcc to get the actual host/target architecture to options: use actual userspace architecture for building in 32bit Jan 24, 2017

sergiusens merged commit 31c45dd into snapcore:master Jan 25, 2017
3 checks passed

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

xenial-amd64 autopkgtest finished (success)
Details

zesty-amd64 autopkgtest finished (success)
Details

@@ -142,7 +141,7 @@ def _npm_install(self, rootdir):
          def _get_nodejs_base(node_engine):
-        -    machine = platform.machine()
+        +    machine = _options._get_platform_architecture()

@@ -81,7 +81,7 @@ def __init__(self, name, options, project):
                  super().__init__(name, options, project)
                  self._npm_dir = os.path.join(self.partdir, 'npm')
                  self._nodejs_tar = sources.Tar(nodejs.get_nodejs_release(
-        -            self.options.node_engine), self._npm_dir)
+        +            self.options.node_engine, self.project), self._npm_dir)

@@ -107,7 +107,7 @@ def __init__(self, name, options, project):
                  super().__init__(name, options, project)
                  self._npm_dir = os.path.join(self.partdir, 'npm')
                  self._nodejs_tar = sources.Tar(get_nodejs_release(
-        -            self.options.node_engine), self._npm_dir)
+        +            self.options.node_engine, self.project), self._npm_dir)

@@ -140,15 +140,15 @@ def _npm_install(self, rootdir):
                      self.run(['npm', 'run', target], cwd=rootdir)
-        -def _get_nodejs_base(node_engine):
-        -    machine = _options._get_platform_architecture()
+        +def _get_nodejs_base(node_engine, project_options):

snapcore/snapcraft

options: use actual userspace architecture for building in 32bit #1060

3v1n0 commented Jan 20, 2017 •

Edited 1 time

3v1n0
Jan 23, 2017

kyrofa commented Jan 20, 2017

3v1n0 changed the title from options: if host is 32bit use the relative backward compatibility architecture to options: use gcc to get the actual host/target architecture Jan 23, 2017

codecov-io commented Jan 23, 2017 •

Edited 1 time

codecov-io
Jan 24, 2017

adconrad commented Jan 23, 2017

3v1n0 commented Jan 23, 2017

3v1n0 changed the title from options: use gcc to get the actual host/target architecture to options: use actual userspace architecture for building in 32bit Jan 24, 2017

sergiusens requested changes Jan 24, 2017

View changes

3v1n0 added some commits Jan 20, 2017

sergiusens reviewed Jan 25, 2017

View changes

sergiusens approved these changes Jan 25, 2017

View changes

sergiusens merged commit `31c45dd` into snapcore:master Jan 25, 2017
3 checks passed

3 checks passed

adconrad commented Jan 25, 2017

adconrad commented Jan 25, 2017

3v1n0 commented Jan 25, 2017 •

Edited 1 time

3v1n0
Jan 25, 2017

kalikiana pushed a commit to kalikiana/snapcraft that referenced this pull request Apr 6, 2017

snapcore/snapcraft

Join GitHub today

options: use actual userspace architecture for building in 32bit #1060

Conversation

3v1n0 commented Jan 20, 2017 • edited Edited 1 time 3v1n0 Jan 23, 2017

kyrofa commented Jan 20, 2017

3v1n0 changed the title from options: if host is 32bit use the relative backward compatibility architecture to options: use gcc to get the actual host/target architecture Jan 23, 2017

codecov-io commented Jan 23, 2017 • edited Edited 1 time codecov-io Jan 24, 2017

Current coverage is 96.34% (diff: 100%)

adconrad commented Jan 23, 2017

3v1n0 commented Jan 23, 2017

3v1n0 changed the title from options: use gcc to get the actual host/target architecture to options: use actual userspace architecture for building in 32bit Jan 24, 2017

sergiusens requested changes Jan 24, 2017 View changes

sergiusens Jan 24, 2017

3v1n0 added some commits Jan 20, 2017

All checks have passed

sergiusens reviewed Jan 25, 2017 View changes

sergiusens Jan 25, 2017

sergiusens Jan 25, 2017

3v1n0 Jan 25, 2017

sergiusens Jan 25, 2017

All checks have passed

sergiusens approved these changes Jan 25, 2017 View changes

Hide details View details sergiusens merged commit 31c45dd into snapcore:master Jan 25, 2017 3 checks passed

3 checks passed

adconrad commented Jan 25, 2017

adconrad commented Jan 25, 2017

3v1n0 commented Jan 25, 2017 • edited Edited 1 time 3v1n0 Jan 25, 2017

kalikiana pushed a commit to kalikiana/snapcraft that referenced this pull request Apr 6, 2017

3v1n0 commented Jan 20, 2017 •

Edited 1 time

3v1n0
Jan 23, 2017

codecov-io commented Jan 23, 2017 •

Edited 1 time

codecov-io
Jan 24, 2017

sergiusens requested changes Jan 24, 2017

View changes

sergiusens reviewed Jan 25, 2017

View changes

sergiusens approved these changes Jan 25, 2017

View changes

sergiusens merged commit `31c45dd` into snapcore:master Jan 25, 2017
3 checks passed

3v1n0 commented Jan 25, 2017 •

Edited 1 time

3v1n0
Jan 25, 2017