veyron/services/mgmt: Fix broken tests with a hack.
veyron/services/mgmt/node/impl started failing the TestAppLifeCycle test after
https://veyron-review.googlesource.com/#/c/5164/ was submitted.
Debugging traced this failure to the innocent change in the packages
being imported in veyron/services/mgmt/node/impl/impl_test.go and the
fact that veyron/lib/testutil calls flag.Parse in its init() method.
The aforementioned change changed the package dependencies of
veyron/services/mgmt/suidhelper/impl. This, in turn, caused the compiler to
change the package initialization order such that veyron/lib/testutil was being
initailized before veyron/services/mgmt/suidhelper/impl.
As a result of this reordering, the flags defined in the impl package were not
available as the flag parsing happened before these new flags were defined.
This change introduces a hack to ensure the correct ordering by adding a
dependency from veyron/lib/testutil to veyron/services/mgmt/suidhelper/flag.
However, this is just a hack (and the fact that the test ever had
a working initialization order was a stroke of luck). The correct fix
is to ensure that flag.Parse it not called in the init of any package
(other than "main").
The correct fix is more involved, so in order to get the build
green again, I propose this temporary hack.
Change-Id: I30cdc14d0d749c205bc9c17b221b213a2df3f8f4
diff --git a/lib/testutil/init.go b/lib/testutil/init.go
index 5e9c28f..9cc119f 100644
--- a/lib/testutil/init.go
+++ b/lib/testutil/init.go
@@ -16,8 +16,11 @@
// define flags that we care about. In practice, this is the
// flags defined by the testing package, the logging library
// and any flags defined by the blackbox package below.
+ // TODO(cnicolau,ashankar): This is painful to ensure. Not calling
+ // flag.Parse in init() is the right solution?
_ "testing"
"time"
+ _ "veyron.io/veyron/veyron/services/mgmt/suidhelper/impl/flag"
// Import blackbox to ensure that it gets to define its flags.
_ "veyron.io/veyron/veyron/lib/testutil/blackbox"
diff --git a/services/mgmt/suidhelper/impl/args.go b/services/mgmt/suidhelper/impl/args.go
index 964f910..34ec8f6 100644
--- a/services/mgmt/suidhelper/impl/args.go
+++ b/services/mgmt/suidhelper/impl/args.go
@@ -8,6 +8,8 @@
"os"
"os/user"
"strconv"
+
+ sflag "veyron.io/veyron/veyron/services/mgmt/suidhelper/impl/flag"
)
type WorkParameters struct {
@@ -31,21 +33,25 @@
const SavedArgs = "VEYRON_SAVED_ARGS"
-var flagUsername, flagWorkspace, flagStdoutLog, flagStderrLog, flagRun *string
-var flagMinimumUid *int64
+var (
+ flagUsername, flagWorkspace, flagStdoutLog, flagStderrLog, flagRun *string
+ flagMinimumUid *int64
+)
func init() {
- // Add flags to global set.
- setupFlags(flag.CommandLine)
+ setupFlags(nil)
}
func setupFlags(fs *flag.FlagSet) {
- flagUsername = fs.String("username", "", "The UNIX user name used for the other functions of this tool.")
- flagWorkspace = fs.String("workspace", "", "Path to the application's workspace directory.")
- flagStdoutLog = fs.String("stdoutlog", "", "Path to the stdout log file.")
- flagStderrLog = fs.String("stderrlog", "", "Path to the stdin log file.")
- flagRun = fs.String("run", "", "Path to the application to exec.")
- flagMinimumUid = fs.Int64("minuid", uidThreshold, "UIDs cannot be less than this number.")
+ if fs != nil {
+ sflag.SetupFlags(fs)
+ }
+ flagUsername = sflag.Username
+ flagWorkspace = sflag.Workspace
+ flagStdoutLog = sflag.StdoutLog
+ flagStderrLog = sflag.StderrLog
+ flagRun = sflag.Run
+ flagMinimumUid = sflag.MinimumUid
}
// ParseArguments populates the WorkParameter object from the provided args
diff --git a/services/mgmt/suidhelper/impl/args_darwin.go b/services/mgmt/suidhelper/impl/args_darwin.go
deleted file mode 100644
index f2f6d00..0000000
--- a/services/mgmt/suidhelper/impl/args_darwin.go
+++ /dev/null
@@ -1,3 +0,0 @@
-package impl
-
-const uidThreshold = 501
diff --git a/services/mgmt/suidhelper/impl/args_linux.go b/services/mgmt/suidhelper/impl/args_linux.go
deleted file mode 100644
index f2f6d00..0000000
--- a/services/mgmt/suidhelper/impl/args_linux.go
+++ /dev/null
@@ -1,3 +0,0 @@
-package impl
-
-const uidThreshold = 501
diff --git a/services/mgmt/suidhelper/impl/args_test.go b/services/mgmt/suidhelper/impl/args_test.go
index 0bacc7c..a209c87 100644
--- a/services/mgmt/suidhelper/impl/args_test.go
+++ b/services/mgmt/suidhelper/impl/args_test.go
@@ -57,7 +57,7 @@
{
[]string{"setuidhelper", "--username", testUserName},
[]string{"A=B"},
- fmt.Errorf("suidhelper does not permit uids less than %d", uidThreshold),
+ fmt.Errorf("suidhelper does not permit uids less than 501"),
WorkParameters{},
},
}
diff --git a/services/mgmt/suidhelper/impl/flag/flag.go b/services/mgmt/suidhelper/impl/flag/flag.go
new file mode 100644
index 0000000..c21e1ca
--- /dev/null
+++ b/services/mgmt/suidhelper/impl/flag/flag.go
@@ -0,0 +1,33 @@
+// Package flag provides flag definitions for the suidhelper package.
+//
+// It does NOT depend on any packages outside the Go standard library.
+// This allows veyron.io/veyron/veyron/lib/testutil to depend on this
+// package, thereby ensuring that the suidhelper flags are defined
+// before the flag.Parse call in testutil.init is made.
+//
+// This is a hack! This file should go away once testutil.init
+// is changed to not parse flags in init().
+// TODO(cnicolaou,ashankar): See above!
+package flag
+
+import "flag"
+
+var (
+ Username, Workspace, StdoutLog, StderrLog, Run *string
+ MinimumUid *int64
+)
+
+func init() {
+ SetupFlags(flag.CommandLine)
+}
+
+func SetupFlags(fs *flag.FlagSet) {
+ Username = fs.String("username", "", "The UNIX user name used for the other functions of this tool.")
+ Workspace = fs.String("workspace", "", "Path to the application's workspace directory.")
+ StdoutLog = fs.String("stdoutlog", "", "Path to the stdout log file.")
+ StderrLog = fs.String("stderrlog", "", "Path to the stdin log file.")
+ Run = fs.String("run", "", "Path to the application to exec.")
+ MinimumUid = fs.Int64("minuid", uidThreshold, "UIDs cannot be less than this number.")
+}
+
+const uidThreshold = 501
