Skip to content

Commit

Permalink
Pass all packages to conversion-gen
Browse files Browse the repository at this point in the history
As per kubernetes#96015 and other reports, conversion-gen does the wrong thing if
dependent packages are not ALSO being re-generated.  It creates new
versions of generated files that have missing conversions.

This change passes all packages as "extras" which will be parsed but not
regenerated (default already does exactly this).
  • Loading branch information
thockin committed Oct 31, 2020
1 parent ad6a2af commit 54e2748
Show file tree
Hide file tree
Showing 4 changed files with 27 additions and 12 deletions.
19 changes: 12 additions & 7 deletions build/root/Makefile.generated_files
Original file line number Diff line number Diff line change
Expand Up @@ -318,8 +318,9 @@ DEFAULTER_DIRS := $(shell \
| xargs -n1 dirname \
| LC_ALL=C sort -u \
)

DEFAULTER_FILES := $(addsuffix /$(DEFAULTER_FILENAME), $(DEFAULTER_DIRS))
DEFAULTER_EXTRA_PEER_PKGS := \
$(addprefix $(PRJ_SRC_PATH)/, $(DEFAULTER_DIRS))

# Reset the list of packages that need generation.
$(shell mkdir -p $$(dirname $(META_DIR)/$(DEFAULTER_GEN)))
Expand All @@ -338,7 +339,7 @@ gen_defaulter: $(DEFAULTER_GEN) $(META_DIR)/$(DEFAULTER_GEN).todo
--v $(KUBE_VERBOSE) \
--logtostderr \
-i "$$pkgs" \
--extra-peer-dirs $$(echo $(addprefix $(PRJ_SRC_PATH)/, $(DEFAULTER_DIRS)) | sed 's/ /,/g') \
--extra-peer-dirs $$(echo $(DEFAULTER_EXTRA_PEER_PKGS) | sed 's/ /,/g') \
-O $(DEFAULTER_BASENAME) \
"$$@"; \
fi
Expand All @@ -359,9 +360,9 @@ $(foreach dir, $(DEFAULTER_DIRS), $(eval \
$(META_DIR)/$(DEFAULTER_GEN).todo: $(DEFAULTER_FILES)

$(DEFAULTER_FILES): $(DEFAULTER_GEN)
if [[ "$(DBG_CODEGEN)" == 1 ]]; then \
if [[ "$(DBG_CODEGEN)" == 1 ]]; then \
echo "DBG: defaulter needed $(@D):"; \
ls -lf --full-time $@ $? || true; \
ls -lf --full-time $@ $? || true; \
fi
echo $(PRJ_SRC_PATH)/$(@D) >> $(META_DIR)/$(DEFAULTER_GEN).todo

Expand Down Expand Up @@ -421,9 +422,12 @@ CONVERSION_DIRS := $(shell \
| xargs -n1 dirname \
| LC_ALL=C sort -u \
)

CONVERSION_FILES := $(addsuffix /$(CONVERSION_FILENAME), $(CONVERSION_DIRS))
CONVERSION_EXTRA_PEER_DIRS := k8s.io/kubernetes/pkg/apis/core,k8s.io/kubernetes/pkg/apis/core/v1,k8s.io/api/core/v1
CONVERSION_EXTRA_PEER_PKGS := \
k8s.io/kubernetes/pkg/apis/core \
k8s.io/kubernetes/pkg/apis/core/v1 \
k8s.io/api/core/v1
CONVERSION_EXTRA_PKGS := $(addprefix $(PRJ_SRC_PATH)/, $(CONVERSION_DIRS))

# Reset the list of packages that need generation.
$(shell mkdir -p $$(dirname $(META_DIR)/$(CONVERSION_GEN)))
Expand All @@ -439,7 +443,8 @@ gen_conversion: $(CONVERSION_GEN) $(META_DIR)/$(CONVERSION_GEN).todo
echo "DBG: running $(CONVERSION_GEN) for $$pkgs"; \
fi; \
./hack/run-in-gopath.sh $(CONVERSION_GEN) \
--extra-peer-dirs $(CONVERSION_EXTRA_PEER_DIRS) \
--extra-peer-dirs $$(echo $(CONVERSION_EXTRA_PEER_PKGS) | sed 's/ /,/g') \
--extra-dirs $$(echo $(CONVERSION_EXTRA_PKGS) | sed 's/ /,/g') \
--v $(KUBE_VERBOSE) \
--logtostderr \
-i "$$pkgs" \
Expand Down
6 changes: 2 additions & 4 deletions hack/verify-generated-files-remake.sh
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,11 @@ source "${KUBE_ROOT}/hack/lib/init.sh"
kube::util::ensure_clean_working_dir

_tmpdir="$(kube::realpath "$(mktemp -d -t verify-generated-files.XXXXXX)")"
kube::util::trap_add "rm -rf ${_tmpdir}" EXIT

_tmp_gopath="${_tmpdir}/go"
_tmp_kuberoot="${_tmp_gopath}/src/k8s.io/kubernetes"
mkdir -p "${_tmp_kuberoot}/.."
cp -a "${KUBE_ROOT}" "${_tmp_kuberoot}/.."

git worktree add "${_tmp_kuberoot}"
kube::util::trap_add "git worktree remove -f ${_tmp_kuberoot} && rm -rf ${_tmpdir}" EXIT
cd "${_tmp_kuberoot}"

# clean out anything from the temp dir that's not checked in
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,11 @@ type CustomArgs struct {
// generator pick up manually written conversion funcs from external packages.
ExtraPeerDirs []string

// Additional dirs to parse and load, but not consider for peers. This is
// useful when packages depend on other packages and want to call
// conversions across them.
ExtraDirs []string

// SkipUnsafe indicates whether to generate unsafe conversions to improve the efficiency
// of these operations. The unsafe operation is a direct pointer assignment via unsafe
// (within the allowed uses of unsafe) and is equivalent to a proposed Golang change to
Expand All @@ -67,6 +72,8 @@ func (ca *CustomArgs) AddFlags(fs *pflag.FlagSet) {
"Comma-separated list of apimachinery import paths which are considered, after tag-specified peers, for conversions. Only change these if you have very good reasons.")
pflag.CommandLine.StringSliceVar(&ca.ExtraPeerDirs, "extra-peer-dirs", ca.ExtraPeerDirs,
"Application specific comma-separated list of import paths which are considered, after tag-specified peers and base-peer-dirs, for conversions.")
pflag.CommandLine.StringSliceVar(&ca.ExtraDirs, "extra-dirs", ca.ExtraDirs,
"Application specific comma-separated list of import paths which are loaded and considered for callable conversions, but are not considered peers for conversion.")
pflag.CommandLine.BoolVar(&ca.SkipUnsafe, "skip-unsafe", ca.SkipUnsafe,
"If true, will not generate code using unsafe pointer conversions; resulting code may be slower.")
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -260,11 +260,13 @@ func Packages(context *generator.Context, arguments *args.GeneratorArgs) generat
continue
}
skipUnsafe := false
extraDirs := []string{}
if customArgs, ok := arguments.CustomArgs.(*conversionargs.CustomArgs); ok {
if len(peerPkgs) > 0 {
peerPkgs = append(peerPkgs, customArgs.BasePeerDirs...)
peerPkgs = append(peerPkgs, customArgs.ExtraPeerDirs...)
}
extraDirs = customArgs.ExtraDirs
skipUnsafe = customArgs.SkipUnsafe
}

Expand Down Expand Up @@ -296,9 +298,12 @@ func Packages(context *generator.Context, arguments *args.GeneratorArgs) generat
for i := range peerPkgs {
peerPkgs[i] = genutil.Vendorless(peerPkgs[i])
}
for i := range extraDirs {
extraDirs[i] = genutil.Vendorless(extraDirs[i])
}

// Make sure our peer-packages are added and fully parsed.
for _, pp := range peerPkgs {
for _, pp := range append(peerPkgs, extraDirs...) {
context.AddDir(pp)
p := context.Universe[pp]
if nil == p {
Expand Down

0 comments on commit 54e2748

Please sign in to comment.