-
Notifications
You must be signed in to change notification settings - Fork 21
Properly package the python script #36
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Gaëtan Lehmann <[email protected]> Signed-off-by: Pau Ruiz Safont <[email protected]>
82cdbd4
to
77821cc
Compare
Signed-off-by: Pau Ruiz Safont <[email protected]>
a8e4bd2
to
f1df55e
Compare
b40c199
to
f6378c7
Compare
|
||
## Installation | ||
|
||
Clone this repository and install the `xcp-ng-dev` package: |
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'm not fully convinced by the xcp-ng-dev
name for a command. It probably lacks an action verb for me. I also find xcp-ng-dev-build
ambiguous because when we run the build env, that's also for building stuff usually. And we "build" software a lot more often than we "build" the container. Not that the old names are better, sure :).
Something that is not quite right at the moment is also the fact that we have a single multipurpose run.py
command. How about adding a mandatory parameter to it that says what action it's supposed to do? start
to start the container without automatically starting a build, and build
to start the container, install the deps, and build. These are the two main uses of the container at the moment. And, translated as installed commands from the package, this would give:
xcp-ng-build-env-run
= start the container (==run.py start
)xcp-ng-dev-build
= start the container, install the deps, do the build (==run.py build
)xcp-ng-build-env-create
= avoiding the word "build" here, but that's actuallybuild.sh
that could probably rename tocreate.sh
.
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.
Maybe shell
would be a more descriptive action than start
in this context? (xcp-ng-build-env-shell
?) (OK it's not an action verb, but maybe still descriptive enough?)
xcp-ng-dev-build = start the container, install the deps, do the build (== run.py build)
did you mean xcp-ng-build-env-build
?
In any case, using separate entrypoints would require some surgery in the previously-run.py
, and #35 already touches quite a bit, maybe this idea can be implemented after that other PR?
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.
xcp-ng-build-env-build
felt awkward so I figured it was not so important to have the same prefix for every command and we could have a shorter one just for the one we use most. And it does perform a build in the context of development. Looks like this idea hasn't seduced you :)
maybe this idea can be implemented after that other PR
Yes, but I think we should at least group "packaging the scripts" and "creating separate entry points". So maybe both after #35?
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.
xcp-ng-build-env-build
felt awkward
Maybe xcpng-buildenv-build
would feel less so (I'm not a big fan of too many hyphens 😉)
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'm not fully convinced by the xcp-ng-dev name for a command. It probably lacks an action verb for me.
It's missing it on purpose: the command can do several things: drop into a shell, or do a local build, for example.
How about adding a mandatory parameter to it that says what action it's supposed to do?
That's what I wanted to explore next, but wanted to restrict the scope of the PR because I haven't used the tool too much.
In general I would like the command to be just one, and feed it actions:xcp-ng-dev shell
, xcp-ng-dev build
xcp-ng-build-env-create = avoiding the word "build" here, but that's actually build.sh that could probably rename to create.sh.
Yes, it's awkward as it is, using env-create might be better, but I do want to keep the common prefix, it's different from the other commands as they have their action separate from the command. So I'll go with xcp-ng-dev-env-create
.
I do think that #35 should be merged before this PR, and others that change the interface.
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.
xcp-ng-dev shell
, xcp-ng-dev build
and xcp-ng-dev-env-create
look like good names to me.
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've renamed build.sh to xcp-ng-dev-env-create
. We can rework xcp-ng-dev later
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.
Personnally, I'd prefer one single big change in how the build env is supposed to be used rather than two successive changes, as in each case people will have to change their habits.
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'll wait to do the rework until the PR for xcp 9 is merged, then.
61d06e3
to
5790db2
Compare
README.md
Outdated
like `pipx install --editable .` | ||
|
||
Using the `--editable` flag will allow you to update the cli tools just by | ||
updating the contents of the git reopsitory, without reinstalling. |
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.
updating the contents of the git reopsitory, without reinstalling. | |
updating the contents of the git repository, without reinstalling. |
README.md
Outdated
or `pipx install .` | ||
|
||
After this, two new commands will be available: `xcp-ng-dev-env-create` and | ||
`xcp-ng-dev`, |
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 sentence ends with a comma
README.md
Outdated
@@ -157,11 +124,11 @@ make | |||
|
|||
If you'd like to develop using the tools on your host and preserve the changes | |||
to source and revision control but still use the container for building, you | |||
can do using by mouning a volume in the container, using the `-v` option to mount | |||
can do using by mounting a volume in the container, using the `-v` option to mount |
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.
No from your change, but as you already fixed something in the sentence, I suggest:
can do using by mounting a volume in the container, using the `-v` option to mount | |
can do so by mounting a volume in the container, using the `-v` option to mount |
... Or any better wording.
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.
Not as atomic as we usually do, but let's go forward
Adds instructions on the readme on how to do it. The name of the main script has been changed from run.py to xcp-ng-d.ev Adds a gitignore to ignore files produces by the packaging Signed-off-by: Pau Ruiz Safont <[email protected]>
This needed a small bit of python glue code Signed-off-by: Pau Ruiz Safont <[email protected]>
This allows the script to be installed and not have to refer to its whole path every time it's invoked.
The name of the installed binary is xcp-ng-dev.
Also fixes a couple of nits I found while trying to run the script
obsoletes #32