New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
packaging/{opensuse,fedora}: allow package build with testkeys included #3450
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3450 +/- ##
==========================================
- Coverage 77.15% 77.14% -0.01%
==========================================
Files 373 373
Lines 25793 25793
==========================================
- Hits 19901 19899 -2
- Misses 4134 4136 +2
Partials 1758 1758
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 with some comments for your consideration
packaging/fedora/snapd.spec
Outdated
%gobuild -o bin/snapctl %{import_path}/cmd/snapctl | ||
%gobuild -o bin/snapd %{import_path}/cmd/snapd | ||
%gobuild -o bin/snap-update-ns %{import_path}/cmd/snap-update-ns | ||
# We have to build snapd first to prevent the build from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment seems to be copy pasted, is this intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed one of both.
packaging/fedora/snapd.spec
Outdated
GOFLAGS="-tags withtestkeys" | ||
%endif | ||
|
||
# We have to build snapd first to prevent the build from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think anything else cares about test keys, if that lets you simplify things, go for it!
packaging/opensuse-42.2/snapd.spec
Outdated
# so we we have to invoke `go install` here manually. | ||
export GOPATH=%{_builddir}/go:%{_libdir}/go/contrib | ||
export GOBIN=%{_builddir}/go/bin | ||
go install -s -v -p 4 -x -tags withtestkeys github.com/snapcore/snapd/cmd/snapd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick, can you use long forms or document the meaning of the switches above in a comment, for the next reader?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean for -x -v -p 4 -x
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
packaging/fedora/snapd.spec
Outdated
GOFLAGS= | ||
%if 0%{?with_test_keys} | ||
GOFLAGS="-tags withtestkeys" | ||
%endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you erasing the GOFLAGS
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
%gobuild -o bin/snapctl %{import_path}/cmd/snapctl | ||
%gobuild -o bin/snapd %{import_path}/cmd/snapd | ||
%gobuild -o bin/snap-update-ns %{import_path}/cmd/snap-update-ns | ||
GOFLAGS= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're still erasing the GOFLAGS here... :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Who else uses GOFLAGS? Nothing in the spec file does and nothing of the RPM golang macros does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Special build with test keys enabled is necessary for a few spread tests.