Open dimethylzinc opened 2 weeks ago
Are you running west update
after changing the manifest? Manifest changes are ignored until you run west update
.
west help compare
usage: west compare [-h] [-a] [--exit-code] [--ignore-branches]
[--no-ignore-branches]
[PROJECT ...]
Compare each project's working tree state against the results
of the most recent successful "west update" command.
.
.
.
It may seem surprising but most west commands don't look at the manifest. Instead, they look at the manifest-rev
branch in each project. That manifest-rev
branch is synchronized every time you run west update
.
Also: could you please provide a short, step-by-step script that reproduces the issue? Just to make sure we're all on the same page.
Are you running west update after changing the manifest? Manifest changes are ignored until you run west update
Not necessarily. And this was the problem we were trying to mitigate. If one developer merges a change to the manifest, another developer might pull that and build without realizing they needed to run west update
. We wanted to use west compare
to detect that and warn that an update was necessary.
We don't want to automatically run west update
before a build because a developer might be deliberately working with some changes in a project which aren't yet committed to the manifest. But automatically running a check and warning is ok.
It may seem surprising but most west commands don't look at the manifest
Exactly. We were surprised by that behavior (though I admit that the behavior is consistent with the explanation in west help compare
once I read it more carefully). I'm guessing there's a good reason not to compare to the manifest?
For us though, it was more useful to compare to against the manifest to catch those changes pulled in remotely. Maybe an option for west compare
to check against the manifest file instead of the manifest-rev
branches?
Also: could you please provide a short, step-by-step script that reproduces the issue? Just to make sure we're all on the same page.
west update
west compare
revision
field of a project
in west.yml
(e.g. to a newer commit)west compare
again
Is west diff --manifest
what you are looking for? It's available in v1.3.0
alpha release.
usage: west diff [-h] [-a] [-m] [PROJECT ...]
Runs "git diff" on each of the specified projects.
Unknown arguments are passed to "git diff".
positional arguments:
PROJECT projects (by name or path) to operate on; defaults to active cloned projects
options:
-h, --help show this help message and exit
-a, --all include output for inactive projects
-m, --manifest show changes relative to the manifest revision
Maybe an option for west compare to check against the manifest file instead of the manifest-rev branches?
Eventually, everyone will want the same option to be duplicated in every command. We need something more generic.
We don't want to automatically run west update before a build because a developer might be deliberately working with some changes in a project which aren't yet committed to the manifest.
From https://github.com/zephyrproject-rtos/west/issues/338#issuecomment-563503380
@tejlmand has separately suggested a west fetch [PROJECT ...] command, which would update manifest-rev in each project without changing the working tree. I agree, for separate reasons not relevant to this issue, that this is a useful command we should have.
Back to you:
If one developer merges a change to the manifest, another developer might pull that and build without realizing they needed to run west update.
I've seen this requested a few times, it's a common issue. For now, developers must learn that all changes to the manifest repo may require a west update
. That's the essence of the manifest after all. Same as git submodules or similar.
I'm guessing there's a good reason not to compare to the manifest?
What you are really asking is "why does the manifest-rev
branch exist?". I don't remember all the rationale but I do remember it was certainly not an "accident". I think one of the reasons was related to import
s. Please scan the documentation and past issues; start with issues linked from #338.
Also, please ask about this and other, related questions above in the #west
channel on Discord. The people who know better should be there.
I think one of the reasons was related to imports [...] Please scan the documentation and past issues; start with issues linked from https://github.com/zephyrproject-rtos/west/issues/338.
This label is another good starting point: https://github.com/zephyrproject-rtos/west/labels/Partial%20imports Look at closed issues too.
What you are really asking is "why does the manifest-rev branch exist?". I don't remember all the rationale but I do remember it was certainly not an "accident". I think one of the reasons was related to imports. Please scan the documentation and past issues; start with issues linked from https://github.com/zephyrproject-rtos/west/issues/338.
OK, that didn't take long: I just searched the project for manifest-rev
and quickly found:
The long story short is: manifest-rev
may look superficially identical to the revision:
field in the manifest, but that is true only in the simplest cases. More complex cases involve import
, branches, etc.
So, you must really consider manifest-rev
as a resolved version which is the output of a potentially complex "computation" performed last time west update
was run. In the general case it's not just a cache of the revision:
field. If you think manifest-rev
is not well described in https://docs.zephyrproject.org/latest/develop/west/index.html then please say so and I'll try to submit a doc fix. I could be wrong but I have vague memories of Marti trying to argue that manifest-rev
was an implementation detail not worth worrying users too much with - if that's true then I think he was wrong ;-)
Now going back to your original question, I think west fetch
would likely be the best past path forward. This would basically run the first part of west update
, including manifest imports, resolution, etc. but it would stop just before changing anything in west projects (apart from the manifest-rev
branch of course). Then maybe we could have something like west compare --fetch
which would simply run west fetch && west compare
. What do you think?
Thanks for looking into this!
@pdgendt west diff --manifest
could well provide us the functionality we're looking for. But does this pose the same problem Marc was talking about, that people will eventually want a --manifest version on every command?
west fetch
does seem like it would be the most robust solution (with or without west compare --fetch
for added convenience). That would work for our use case and I think for more complex ones too..
I guess our current extension works for us (for now) because we're only using simple cases where the resolution of the manifest is trivial.
Is
west diff --manifest
what you are looking for?
So west diff --manifest
does indeed "solve" @dimethylzinc's problem for now by passing the revision:
field "as is" to git diff
. This lack of manifest-rev
and inconsistency with other commands surprised me so I took a closer look and... I quickly found one bug: west diff --manifest
fails when the manifest points to (remote[*]) branches!
Fix submitted (and inconsistency removed) in #748, please review. Please don't shoot the messenger!
[*] the manifest cannot point to local branches.
Initial title: "west compare doesn't detect updated project revisions in manifest"
When running
west compare
, I expected that if my repo state didn't match the manifest, it would report it.Actual behaviour: changes to repo are detected. But changes to manifest (e.g. change sha in a project's
revision
property) are not detected.Is this the intended behaviour?
In our project, we want to confirm before building that repo state matches the manifest. We do this by running
west compare
before a build. But this doesn't catch an updated manifest. We have currently worked around this with a west extension which useswest compare
and also checks each project repo in the workspace against the revision in the manifest.Would an update to
west compare
to include this check be useful to others?cc:
448
548