Skip to content

Allow use of docker/lint.sh clang_format with git-worktree#8639

Closed
areusch wants to merge 2 commits into
apache:mainfrom
areusch:docker-script-improvements
Closed

Allow use of docker/lint.sh clang_format with git-worktree#8639
areusch wants to merge 2 commits into
apache:mainfrom
areusch:docker-script-improvements

Conversation

@areusch

@areusch areusch commented Aug 3, 2021

Copy link
Copy Markdown
Contributor
  • mount repo at $(pwd) by default; fixes problems when using git-worktree.
  • allows usage of docker/lint.sh with arbitrary tvm repo

cc @jroesch @mikepapadim @tqchen

@areusch areusch changed the title improve docker/bash.sh workflow Allow use of docker/lint.sh clang_format with git-subtrees Aug 3, 2021
@areusch areusch changed the title Allow use of docker/lint.sh clang_format with git-subtrees Allow use of docker/lint.sh clang_format with git-worktree Aug 3, 2021
@tqchen

tqchen commented Aug 3, 2021

Copy link
Copy Markdown
Member

@areusch let us do a quick check in case some of the CI might depend on the /workspace behavior

@areusch

areusch commented Aug 3, 2021

Copy link
Copy Markdown
Contributor Author

@tqchen last time it was the rust rebuild which is conveniently disabled now :)

@areusch

areusch commented Aug 3, 2021

Copy link
Copy Markdown
Contributor Author

not that i am trying to actively break rust but i don't understand the build system enough to know how to fix it and last time the PR languished forever and died because no one was willing to help.

 * properly quote command-line arguments
 * mount repo at $(pwd) by default; fixes problems when using git-worktree.
@areusch areusch force-pushed the docker-script-improvements branch from 9050f57 to 339ebda Compare August 4, 2021 05:55

@leandron leandron left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

* mount repo at $(pwd) by default; fixes problems when using git-worktree.

What is the problem when using git-worktree? I'm not very familiar with it.

* allows usage of docker/lint.sh with arbitrary tvm repo

Maybe we could add a "usage" example in the header of the script, to instruct people on how to do that?

Also, a bit off topic for here but I think we really should think about processing the inputs in a loop so that users can provide non-positional parameters in any order.

Comment thread docker/bash.sh Outdated
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
WORKSPACE="$(pwd)"

if [ "$1" == "--repo-mount-point" ]; then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to add this on line 23 small documentation to the command line, also to show the order in which parameters need to be provided.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@areusch

areusch commented Aug 4, 2021

Copy link
Copy Markdown
Contributor Author

@leandron try running docker/bash.sh ci_cpu git status from a git-worktree root that's not the original cloned repository. it will fail because it can't access the original repo (the original repo path is placed as a relative path in .git). by mounting the repo based on the pwd, this issue is worked around.

Maybe we could add a "usage" example in the header of the script, to instruct people on how to do that?

ack

Also, a bit off topic for here but I think we really should think about processing the inputs in a loop so that users can provide non-positional parameters in any order.

will see if i can do this in next patch

@areusch

areusch commented Aug 10, 2021

Copy link
Copy Markdown
Contributor Author

superseded by #8670

@areusch areusch closed this Aug 10, 2021
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.

3 participants