Skip to content
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

docs: fix docs for #6776 #6798

Merged
merged 2 commits into from Apr 14, 2015
Merged

docs: fix docs for #6776 #6798

merged 2 commits into from Apr 14, 2015

Conversation

xiang90
Copy link
Contributor

@xiang90 xiang90 commented Apr 14, 2015

./hack/verify-gendocs.sh fails after 2a9141f on darwin/amd64.

run ./hack/run-gendocs.sh to fix it.

Well... After I ran ./hack/run-gendocs.sh it is fixed on my darwin, but it is broken on linux build...

@yifan-gu
Copy link
Contributor

Strange. It's building on my machine, (ubuntu)

@yifan-gu
Copy link
Contributor

And the commits seems odd, it just moves several lines around...

@xiang90
Copy link
Contributor Author

xiang90 commented Apr 14, 2015

@yifan-gu It seems to be a darwin thing...

/cc @dchen1107

@j3ffml
Copy link
Contributor

j3ffml commented Apr 14, 2015

This is passing at head, so you probably just need to run make clean and try rebuilding.

@xiang90
Copy link
Contributor Author

xiang90 commented Apr 14, 2015

@jlowdermilk Have you tried it on Darwin? It passes on linux, but not on my mac... wired...

@erictune
Copy link
Member

Are you sure make clean is working? Sometimes boot2docker vm is wedged and
make clean does not succeed.

On Tue, Apr 14, 2015 at 10:28 AM, Xiang Li notifications@github.com wrote:

@jlowdermilk https://github.com/jlowdermilk Have you tried it on
Darwin? It passes on linux, but not on my mac... wired...


Reply to this email directly or view it on GitHub
#6798 (comment)
.

@j3ffml
Copy link
Contributor

j3ffml commented Apr 14, 2015

I'm getting the same error on Darwin too:

hack/verify-gendocs.sh
+++ [0414 11:12:52] Building go targets for darwin/amd64:
    cmd/gendocs
    cmd/genman
    cmd/genbashcomp
+++ [0414 11:12:53] Placing binaries
diffing /Users/jeffml/kubernetes/docs/ against freshly generated docs
diff -Naupr -I 'Auto generated by' /Users/jeffml/kubernetes/docs/.files_generated /Users/jeffml/kubernetes/docs_tmp/.files_generated
--- /Users/jeffml/kubernetes/docs/.files_generated  2015-04-14 11:12:45.000000000 -0700
+++ /Users/jeffml/kubernetes/docs_tmp/.files_generated  2015-04-14 11:12:55.000000000 -0700
@@ -1,3 +1,4 @@
+kubectl.md
 kubectl_api-versions.md
 kubectl_cluster-info.md
 kubectl_config.md
@@ -16,7 +17,6 @@ kubectl_expose.md
 kubectl_get.md
 kubectl_label.md
 kubectl_log.md
-kubectl.md
 kubectl_namespace.md
 kubectl_port-forward.md
 kubectl_proxy.md
diff -Naupr -I 'Auto generated by' /Users/jeffml/kubernetes/docs/man/man1/.files_generated /Users/jeffml/kubernetes/docs_tmp/man/man1/.files_generated
--- /Users/jeffml/kubernetes/docs/man/man1/.files_generated 2015-04-14 11:12:45.000000000 -0700
+++ /Users/jeffml/kubernetes/docs_tmp/man/man1/.files_generated 2015-04-14 11:12:53.000000000 -0700
@@ -1,14 +1,13 @@
-kubectl.1
 kubectl-api-versions.1
 kubectl-cluster-info.1
-kubectl-config.1
-kubectl-config-set.1
 kubectl-config-set-cluster.1
 kubectl-config-set-context.1
 kubectl-config-set-credentials.1
+kubectl-config-set.1
 kubectl-config-unset.1
 kubectl-config-use-context.1
 kubectl-config-view.1
+kubectl-config.1
 kubectl-create.1
 kubectl-delete.1
 kubectl-describe.1
@@ -26,3 +25,4 @@ kubectl-run-container.1
 kubectl-stop.1
 kubectl-update.1
 kubectl-version.1
+kubectl.1
/Users/jeffml/kubernetes/docs/ is out of date. Please run hack/run-gendocs.sh

The problem is that sort doesn't behave the same on linux and darwin. @filbranden, any ideas how to make this line do the same thing on darwin/linux? If not, I might just change it to inline python.

@j3ffml
Copy link
Contributor

j3ffml commented Apr 14, 2015

cc @eparis

@filbranden
Copy link
Contributor

Not sure this is actually the problem (and I don't really have a darwin close by), but when using sort, ALWAYS use LC_COLLATE=C sort since otherwise collation order depends on the environment's locale. I could easily believe the default locale on MacOS X is different from the default one on Linux...

@filbranden
Copy link
Contributor

If you have a darwin around, try these two commands and compare their output:

$ { echo kubectl.1; echo kubectl-api-versions.1; } | sort
kubectl-api-versions.1
kubectl.1

$ { echo kubectl.1; echo kubectl-api-versions.1; } | LC_COLLATE=C sort                                                                                                                      
kubectl-api-versions.1
kubectl.1

If the former looks inverted and the latter looks the same, then LC_COLLATE is really what you want here.

@smarterclayton
Copy link
Contributor

On my Yosemite build those are in the same order. But I have LANG="en_US.UTF-8" set.

@eparis
Copy link
Contributor

eparis commented Apr 14, 2015

On my linux box:

$ { echo kubectl.1; echo kubectl-api-versions.1; } | sort
kubectl.1
kubectl-api-versions.1

$ { echo kubectl.1; echo kubectl-api-versions.1; } | LC_COLLATE=C sort
kubectl-api-versions.1
kubectl.1

@j3ffml
Copy link
Contributor

j3ffml commented Apr 14, 2015

@filbranden, they look the same on darwin, but they differ on linux:

$ { echo kubectl.1; echo kubectl-api-versions.1; } | sort
kubectl.1
kubectl-api-versions.1

$ { echo kubectl.1; echo kubectl-api-versions.1; } | LC_COLLATE=C sort                                                                                                                      
kubectl-api-versions.1
kubectl.1

Looks like LC_COLLATE is what we want. @xiang90, can we use this PR to make the change? the command that needs to be updated is here.

@xiang90
Copy link
Contributor Author

xiang90 commented Apr 14, 2015

@jlowdermilk Sure thing. Wait a moment. need to have lunch right now.

@filbranden
Copy link
Contributor

@jlowdermilk Ah yes I forgot that on my Linux I always set LC_COLLATE=C so I always get sane behavior from sort in all cases...

Indeed using LC_COLLATE=en_US.UTF-8 sort gives me the wrong sort order as @eparis just confirmed.

In any case, scripts should always force LC_COLLATE=C sort if you're using sort for programmatic purposes to isolate from the environment's locale...

Might as well just go through all the *.sh scripts in Kubernetes looking for calls to sort and fixing them all to do the right thing...

Cheers,
Filipe

@xiang90
Copy link
Contributor Author

xiang90 commented Apr 14, 2015

@jlowdermilk Hrm... still does not work well after the change.

@eparis
Copy link
Contributor

eparis commented Apr 14, 2015

@xiang90 it looks good to me, what's wrong?

@xiang90
Copy link
Contributor Author

xiang90 commented Apr 14, 2015

@eparis Travis and Shippable failed. :(

@eparis
Copy link
Contributor

eparis commented Apr 14, 2015

for a more readable commit, you can git checkout docs/kubectl*.md

@filbranden the man page for sort suggest LC_ALL=C

Is that what we should be using?

@eparis
Copy link
Contributor

eparis commented Apr 14, 2015

Note that setting only 'LC_COLLATE' has two problems.
First, it is ineffective if 'LC_ALL' is also set. Second, it has
undefined behavior if 'LC_CTYPE' (or 'LANG', if 'LC_CTYPE' is unset) is
set to an incompatible value. For example, you get undefined behavior
if 'LC_CTYPE' is 'ja_JP.PCK' but 'LC_COLLATE' is 'en_US.UTF-8'.

@filbranden
Copy link
Contributor

@eparis Yes, I think you're right setting LC_ALL is probably better since it's the strongest.

(I thought of it first, rejected thinking it was using a cannon to kill a fly, but on second thought it makes more sense.)

Let's go with LC_ALL=C sort ... and I'd recommend checking all the *.sh files in the tree for uses of sort and trying to fix them all (probably not in this same PR, or if in this same PR then in a separate commit) since they're potentially all affected.

Cheers,
Filipe

@xiang90
Copy link
Contributor Author

xiang90 commented Apr 14, 2015

@filbranden @jlowdermilk @eparis Fixed.

I will try to "fix" all sh in another pr.

@j3ffml
Copy link
Contributor

j3ffml commented Apr 14, 2015

lgtm

j3ffml added a commit that referenced this pull request Apr 14, 2015
@j3ffml j3ffml merged commit b59266a into kubernetes:master Apr 14, 2015
@eparis
Copy link
Contributor

eparis commented Apr 14, 2015

@filbranden I'm not as smart are my last comment, I'm just quoting from the footnotes in info coreutils 'sort invocation'

@filbranden
Copy link
Contributor

@eparis Which is actually the smart thing to do: RTFM! :-)

@xiang90 xiang90 deleted the fix_doc branch April 14, 2015 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants