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
Get west and manifest revisions from a configuration file + support any revision format + misc. #75
Conversation
ulfalizer
commented
Oct 9, 2018
•
edited
edited
d24ed28
to
fdf8229
Compare
To be consistent with how Git behaves:
On Windows, |
Got #64 rebased on top of this now. I'll add back the hint there too, with some simple system for context-dependent error messages ("git command foo failed while doing bar"). |
As discussed on IRC, consistency with git is a go. |
@mbolivar other than the file locations, are you happy with the changes? |
Pushed a new version that uses more Git-like paths. I followed the |
src/west/config.py
Outdated
|
||
# User-specific and repository-specific | ||
|
||
files.append(os.path.expanduser('~/.westconfig')) |
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.
will this work on Windows?
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.
It does
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.
Yep, works on Windows too: https://docs.python.org/3/library/os.path.html#os.path.expanduser
The Python docs are pretty awesome. :)
@@ -199,6 +199,9 @@ def main(argv=None): | |||
argv = sys.argv[1:] | |||
args, unknown = parse_args(argv) | |||
|
|||
# Read the configuration files |
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.
Should we try to create one for those using the old bootstrap?
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.
You mean people who have already bootstrapped?
Defaults are used if the configuration file doesn't exist, so shouldn't need it.
9e6b08a
to
d970ed5
Compare
This looks fine to me as-is now. That said, you introduced some additional changes that were unrelated to the config file in the same commit from what I see. Would be good if you could keep those in separate commits in upcoming PRs. |
src/west/config.py
Outdated
# on all systems. They're listed in git-config(1). | ||
files.append(os.path.expandvars('$XDG_CONFIG_HOME/git/config') | ||
if 'XDG_CONFIG_HOME' in os.environ else | ||
os.path.expanduser('~/.config/git/config')) |
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.
s/git/west
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.
Heh... whoops :)
Fixed.
Tested on Windows, works well except for a typo I commented on. |
@nashif copied you as per making 0.3.0 happen soon. |
src/west/config.py
Outdated
if platform.system() == 'Linux': | ||
files = ['/etc/westconfig'] | ||
elif platform.system() == 'Darwin': # Mac OS | ||
# This was seen on Carles' machine ($(prefix) = /usr/local) |
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.
missed this, please remove Carles
and say "local machine"
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.
Done
src/west/config.py
Outdated
# This was seen on Carles' machine ($(prefix) = /usr/local) | ||
files = ['/usr/local/etc/westconfig'] | ||
elif platform.system() == 'Windows': | ||
# Seen on Carles' machine. Not sure how it got derived though. |
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.
missed this, please remove Carles
and say "local machine"
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.
Done
I think a good discussion is already ongoing with regards to west configurations options. If more things are added to same PR, the discussion has a risk of getting more noisy than it already is. Although I do think the |
@tejlmand
|
Oh, and nothing rebases cleanly anymore, since there's so much stuff in here, so that's the real reason. Getting some of this in earlier would've been nice. I'm not the kind of guy who abandons stuff once it's in. |
Added some (That one probably does rebase cleanly. I'd just forgotten you can open separate PRs at this point. ;) |
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.
This Pull Request is dragging for too long and I think some of the discussions are too speculative at this point.
Without the URL being a part of the config file, having the bootstrapper read the config file is useless, because then the user can still not set the URL in the user or system-wide variables to know where west init
should pull the manifest and west from.
I still believe that we should have the URL in the configuration file, and that the bootstrapper should be able to load the configuration file, but at this point we do not have enough feedback from users to back my (or anyone else's) arguments. This is an issue because we end up getting caught in eternal discussions about what the user would like or not to do, what the user is likely to want or not. And we cannot realistically know that yet is my opinion.
Hence, I would propose we merge this, publish 0.3.0 and start collecting feedback from users and their workflow with west so we can actually make informed decisions that reflect the thinking of the majority of users. This includes as well designing the workflow that works best with west and the manifest, so that we know exactly how users work with the manifest, how they fork the repo and how they manage their separate forests (as in set of trees).
Let the users create issues and enhancement/change requests so that they, and not us, can steer the course of development of this tool.
Heh... spoke too soon. It didn't rebase cleanly either. |
I don't see how adding |
Agree with @carlescufi here. |
Add configuration file support via the standard Python configparser module, which uses a Git-like .ini format. Use it to save the west and manifest revisions specified when running 'west init', and use the saved revisions when updating the west and manifest repositories. Configuration file paths are based on Git, for consistency with the Git-like commands. See the FILES section in git-config(1). Mac OS and Windows paths were checked by Carles Cufi with 'git config --list --show-origin'. Add some new shorthands to _expand_shorthands() in project.py to factor out some logic while removing the hardcoding of the west and manifest upstream branches. The qual-* versions of shorthands give the full path to refs (refs/heads/foo, remotes/origin/bar, etc.) Fixes: zephyrproject-rtos#63 Fixes: zephyrproject-rtos#67 Signed-off-by: Ulf Magnusson <Ulf.Magnusson@nordicsemi.no>
Switch to a simpler and more flexible approach for dealing with upstream revisions, that works with any revision format (SHA, branch, qualified ref (e.g. refs/heads/foo), or (annotated) tag): 1. Fetch (just) the upstream revision, exactly as specified. 2. Dereference it to a commit via FETCH_HEAD, and point 'manifest-rev' at it. Using FETCH_HEAD makes it possible to apply ^{commit} peeling (see git-rev-parse(1)), which makes things work for e.g. annotated tags too (git tag -a ...). A regression (could be an improvement, depending on how you see it) is that just the upstream revision is fetched now. It'd be simple to do a full fetch if we'd want that later though. Signed-off-by: Ulf Magnusson <Ulf.Magnusson@nordicsemi.no>
The old _fetch() behavior was to check out a detached HEAD at 'manifest-rev' if the repository was initialized in the same call to _fetch(). This meant that no detached HEAD was checked out if the initial fetch was aborted and then resumed. Instead, always check out a detached head at 'manifest-rev' if nothing is checked out in the repository (which is almost guaranteed to mean that we're still in the 'git init' state). That makes aborting and resuming safe. Nothing being checked out can be detected by looking at HEAD, which will point to a non-existing ref. Fixes: zephyrproject-rtos#78 Signed-off-by: Ulf Magnusson <Ulf.Magnusson@nordicsemi.no>
To be consistent with repo (though I haven't actually checked the behavior myself). Fixes: zephyrproject-rtos#73 Signed-off-by: Ulf Magnusson <Ulf.Magnusson@nordicsemi.no>
Based on the corresponding git option (see git-config(1)). 'true'/'false' are the only supported values at the moment, but they work the same as in git (output is colorized if color.ui = true and the output file is a terminal). If color.ui isn't specified, it defaults to true. color.ui is not written to west/config, since it makes more sense as a global option than a per-repository option. It can still be disabled in west/config though, though ~/.westconfig would make more sense. I was thinking of toggling colors at a higher level with colorama at first, but colorama.init() makes it a bit tricky to get right for both Linux and Windows. Probably not too bad to make it explicit like this. Add a _reset_colors() helper to log.py as well to factor out a tricky and easily typo'd print(). Signed-off-by: Ulf Magnusson <Ulf.Magnusson@nordicsemi.no>
Yeah, you can add lazy bastard to the list of excuses. That one might not have been horrible to be honest, though I haven't checked. I've changed the |
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.
Approving to get this rolling.
Although I still think the whole discussion on the conf file is very important, and should be taken later, I don't see that discussion as something which should block a v0.3.0 release.
so aligned with @carlescufi
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.
Edit: deleted my original comment, I think this was user error, retesting
src/west/_bootstrap/main.py
Outdated
os.path.join(directory, WEST_DIR, WEST)) | ||
|
||
clone(args.manifest_url, args.manifest_rev, | ||
clone('manifest repisitory', args.manifest_url, args.manifest_rev, |
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.
Small nit: "repisitory" typo
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.
Fixed
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 tested and checking out a revision which is an annotated tag is working for me. Thanks for the fix! That's awesome.
I think there is a bug in the "update" behavior, though. The update command seems to be rebasing on top of the remote "master" rather than the revision in the config file, but I can't see why that is.
Here are a pair of testing trees in my github manifest and west forks:
https://github.com/mbolivar/zephyr-manifest/tree/wip
https://github.com/mbolivar/west/tree/pr-75
Note that both of these branches (wip and pr-75) lag behind the master branch in my github forks.
Steps to reproduce the behavior I described above:
# Fetch and initialize manifest repositories:
$ git clone https://github.com/mbolivar/west /tmp/west
$ git clone https://github.com/mbolivar/zephyr-manifest /tmp/manifest
$ (cd /tmp/west; git checkout pr-75)
$ (cd /tmp/manifest; git checkout wip)
# Set up initial wworkdir:
$ west init -b file:///tmp --wr pr-75 --mr wip wwork
$ cd wwork
# Check that initial configuration file is as expected:
$ cat west/config
[west]
remote = origin
revision = pr-75
[manifest]
remote = origin
revision = wip
# Confirm that west and manifest repositories are initialized
# correctly to the expected revisions, and that they are up to date
# with the origin revisions:
$ git --no-pager -C west/west log -1 --oneline
1e16495 (HEAD -> pr-75, origin/pr-75, origin/HEAD) bootstrap: Print some information during initalization
$ git --no-pager -C west/manifest log -1 --oneline
0a3ef22 (HEAD -> wip, origin/wip, origin/HEAD) default.yml: delete unused default revision
# The following west update shouldn't do anything: both repositories
# are up to date with their corresponding remote revisions. But it
# does!
$ west update
First, rewinding head to replay your work on top of it...
Applying: commands: projects: Put '--' before URL in 'git remote add'
Applying: Rename 'list-projects' to 'list'
Applying: Get west and manifest revisions from a configuration file
Applying: commands: project: Support any kind of revision format
Applying: commands: project: Improve behavior for an aborted initial 'fetch'
Applying: commands: project: Use remote name from manifest instead of 'origin'
Applying: Add color.ui option for turning colors on/off
Applying: Make configuration values always available
Applying: commands: project: Allow West/manifest remote to be changed
Applying: commands: project: Check if projects are up-to-date when rebasing
Applying: commands: project: Give context-specific errors for failed self-updates
Applying: commands: project: Refactor self-update a bit
Applying: commands: project: Add a 'west clone' command
Applying: bootstrap: Print some information during initalization
=== Updated (rebased) west (west/west/) to the latest version
First, rewinding head to replay your work on top of it...
Applying: tag zephyr
Applying: default.yml: delete unused default revision
Using index info to reconstruct a base tree...
M default.yml
Falling back to patching base and 3-way merge...
Auto-merging default.yml
CONFLICT (content): Merge conflict in default.yml
error: Failed to merge in the changes.
Patch failed at 0002 default.yml: delete unused default revision
Use 'git am --show-current-patch' to see the failed patch
Resolve all conflicts manually, mark them as resolved with
"git add/rm <conflicted_files>", then run "git rebase --continue".
You can instead skip this commit: run "git rebase --skip".
To abort and get back to the state before "git rebase", run "git rebase --abort".
FATAL ERROR: Command 'git rebase 'FETCH_HEAD^{commit}'' failed for manifest (west/manifest/), while running automatic self-update. Please fix the state of the repository, or pass --no-update to 'west fetch/pull' to skip updating the manifest and West for the duration of the command.
This is nice in case something tries to read a configuration value before the configuration files have been read. This could happen for the logging functions for example, and for tests. The configuration is empty before the configuration files have been read, so defaults must be provided. They must be anyway though, for the code to be robust in case configuration files are missing. Signed-off-by: Ulf Magnusson <Ulf.Magnusson@nordicsemi.no>
Store the name of the remote used for West and manifest updates in west/config, like Git does it. That way the remote can be switched (after adding a remote). Another option would be to store a URL, but it's less flexible and would add hairy remote management code to rewrite remote URLs. It'd also make it impossible for users to manually manage remotes in Git, as their settings would get overwritten. We could probably get rid of the Remote.url field. That information is already available in the Git repository (we could fetch it from there if we want to show it), and we should respect the repository settings in case they have been changed by the user. Signed-off-by: Ulf Magnusson <Ulf.Magnusson@nordicsemi.no>
_update() updates the special west/ and manifest/ repositories and has code for checking if HEAD contains all the commits from a particular revision (is up-to-date re. the revision). Move that code into a separate _up_to_date_with() function, and also use it when rebasing normal projects. This gives a less spammy message when running 'pull' and 'rebase' on already up-to-date projects: $ west rebase === zephyr (zephyr/) is up-to-date with manifest-rev This commit also removes the special error message when self-updating fails, returning to plain _git() with check=True. A separate context hint mechanism will be added after this commit instead. Signed-off-by: Ulf Magnusson <ulfalizer@gmail.com>
Add a context manager ('with foo:' thingy) for setting a context for a block of code. The context is just some text that gets added to the error whenever a Git command fails. Use the context manager to point out whenever a failing Git command is related to self-updates. That could be a bit tricky to debug otherwise. Example error: FATAL ERROR: Command 'git rebase 'FETCH_HEAD^{commit}'' failed for manifest (west/manifest/), while running automatic self-update. Please fix the state of the repository, or pass --no-update to 'west fetch/pull' to skip updating the manifest and West for the duration of the command. Signed-off-by: Ulf Magnusson <ulfalizer@gmail.com>
Just have two _update_manifest() and _update_west() functions and get rid of the flags. Bit easier to follow. _update() was a bit overly generic as a name too. Signed-off-by: Ulf Magnusson <ulfalizer@gmail.com>
This is just a shorthand for 'west fetch' + 'west checkout -b <branch>'. The name of the branch is based on the revision from the manifest. For revisions that are qualified refs (refs/heads/foo), the last component (foo) is used as the name. For SHA revisions, it doesn't make much much sense to name the branch after the SHA, so 'work' is used instead. The name of the created branch(es) can be overriden by passing '-s <branch name>' to 'west clone'. Signed-off-by: Ulf Magnusson <ulfalizer@gmail.com>
Makes it a bit clearer what's going on, and helps people find the configuration file. Example output: Initializing in /home/ulf/westwest === Cloning west repository from https://github.com/zephyrproject-rtos/west, rev. master === Cloning into '/home/ulf/westwest/west/west'... remote: Enumerating objects: 803, done. remote: Total 803 (delta 0), reused 0 (delta 0), pack-reused 803 Receiving objects: 100% (803/803), 239.55 KiB | 371.00 KiB/s, done. Resolving deltas: 100% (428/428), done. === Cloning manifest repisitory from https://github.com/zephyrproject-rtos/manifest, rev. master === Cloning into '/home/ulf/westwest/west/manifest'... remote: Enumerating objects: 6, done. remote: Counting objects: 100% (6/6), done. remote: Compressing objects: 100% (6/6), done. remote: Total 25 (delta 0), reused 2 (delta 0), pack-reused 19 Unpacking objects: 100% (25/25), done. === Initial configuration written to /home/ulf/westwest/west/config === === West initialized === Maybe some helpful hint could be added to the end too. Signed-off-by: Ulf Magnusson <ulfalizer@gmail.com>
@mbolivar Had goofed up in |
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 confirm the fix, thanks @ulfalizer.
I am in the same boat as @carlescufi and @tejlmand about west clone
and everything else :)