fetch: use proper appc os/arch labels #3621
Conversation
|
Can one of the admins verify this patch? |
|
+1 |
common/common.go
Outdated
| arch += string(b) | ||
| } | ||
| } | ||
| return arch |
There was a problem hiding this comment.
I think it makes more sense to use ToAppcOSArch directly here.
There was a problem hiding this comment.
First I wanted to use this, but I don't know a way of getting the arm-flavor (5,6,7...) from go. I think, I could use the runtime/internal/sys GoarchArm constant, but I can not use internal packages. If you know a way of getting this information I would be very happy to use the ToAppcOSArch function.
There was a problem hiding this comment.
This information should be available at runtime as the runtime.goarm symbol, but it is private. I don't have any ARM device to test, but can you quickly check if something like https://gist.github.com/lucab/f7162ca2d95191c692edc12ea8ccaaef works to get the value?
There was a problem hiding this comment.
Strange, your code works but the result is not as I expected:
pi@bb-test-north:~ $ uname -a
Linux bb-test-north 4.4.17-v7+ #904 SMP Tue Aug 16 17:02:00 BST 2016 armv7l GNU/Linux
pi@bb-test-north:~ $ go run arm-test.go
6
pi@bb-test-north:~ $ GOARM=6 go run arm-test.go
6
pi@bb-test-north:~ $ GOARM=7 go run arm-test.go
7
Go assumes at compile time that you are going to use a armv6l device (If you dont specify something else), since so the code can be executed on armv6l and armv7l devices. Dave Cheney says here that there is not much benefit from compiling especially for armv7l devices.
I don't know if this should be the source of truth. The syscall I made is safer I think.
But by the way, your gist is very smart. I didn't even know about the possibility to instruct the linker to do such things. Is this documented somewhere?
There was a problem hiding this comment.
I don't know if this should be the source of truth
This is a tricky topic as in fact there isn't a well-shared ABI for ARMv7. You may read more at https://wiki.debian.org/ArmHardFloatPort, but in short the FPU part of the ABI is not fixed and is known to have incompatible differences (eg. RaspPi and BeagleBone are both armv7l but binaries are not 100% portable). This resulted in so much frustation and bugs, that I mostly agree with golang decision here of defaulting to armv6. For your PR, I think that matching rkt architecture is fine here for the general case, and advanced users may tweak the whole thing (rkt + images) if they need to, provided they know the troubles they are stepping into.
I didn't even know about the possibility to instruct the linker to do such things. Is this documented somewhere?
It is documented here. I found it while looking for linker-specific tricks.
rkt/fetch.go
Outdated
| @@ -27,12 +28,12 @@ import ( | |||
| ) | |||
|
|
|||
| const ( | |||
| defaultOS = runtime.GOOS | |||
| defaultArch = runtime.GOARCH | |||
| defaultOS = runtime.GOOS | |||
There was a problem hiding this comment.
Would you mind introducing a similar GetOS() here?
There was a problem hiding this comment.
I can do that, but it will be a simple "return runtime.GOOS" function, so useless except that it looks similar to GetArch().
|
Thanks for this PR! Fix looks ok in general, but I'd suggest re-using the existing app helper. Also, would you mind formatting the commit message as per https://github.com/coreos/rkt/blob/master/CONTRIBUTING.md#format-of-the-commit-message? |
trusch
force-pushed
the
master
branch
2 times, most recently
from
17546e9
to
5eef1d7
Compare
…h" working on arm devices GetArch() returns the architecture of the current system, and especially returns either armv6l, armv7l or aarch64 if on arm devices (GOARCH is always arm). This leads to working rkt fetch on arm devices.
|
@lucab I'm now using the ToAppcOSArch() function in conjunction with runtime.goarm as you suggested. Fortunatly rkt's build process already detects armv6l / armv7l and sets GOARM accordingly, therefore an armv7l device will require an armv7l arch label. I also stuffed my work into a single commit, so it will be cleaner to merge if you accepts this PR. |
|
ok to test |
lucab
changed the title
| @@ -486,3 +487,30 @@ func GetExitStatus(err error) (int, error) { | |||
| } | |||
| return -1, err | |||
| } | |||
|
|
|||
| // reference to GOARM, needs to be globally, if in GetArch() function it resolves to zero | |||
| //go:linkname goarm runtime.goarm | |||
There was a problem hiding this comment.
According to the doc, you should also import "unsafe" for this annotation to be effective.
There was a problem hiding this comment.
unsafe is already imported.
There was a problem hiding this comment.
Nevermind, GH code-folding tricked me.
common/common.go
Outdated
| // and armv6l, armv7l, arm64 otherwise | ||
| func GetArch() string { | ||
| arch := runtime.GOARCH | ||
| switch arch { |
There was a problem hiding this comment.
it should be safe to do this appc lookup for all architectures, not just for arm.
|
I tested these patches and they seem to fix rkt when fetching arm64 aci images. |
|
@glevand For what architectures can you confirm this? I can only test on a armv7l device (and amd64 of course). Do you have some armv6l or aarch64 devices to test? I could provide test ACI's if this would be helpfull. |
|
@trusch I only test on arm64. I recommend you use QEMU if you want to test others. |
|
@lucab rkt can't fetch ACI images without this. Is it ready to merge? |
|
Can one of the admins verify this patch? |
|
ok to test |
added function GetArch() to common/common.go which replaces runtime.GOARCH-calls in fetch related code. GetArch() returns the architecture of the current system, and especially returns either armv6l, armv7l or aarch64 if on arm devices (GOARCH is always arm). This leads to working rkt fetch on arm devices.