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.

Sign up for GitHub

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

Jump to bottom

Use manifest imports #1942

Merged
merged 1 commit into from
Mar 26, 2020
Merged

Use manifest imports #1942

merged 1 commit into from
Mar 26, 2020

Conversation

mbolivar
Copy link

@mbolivar mbolivar commented Feb 21, 2020 •
edited
Loading

Use the west 0.7 manifest imports feature in the NCS. This means we now require west 0.7. I will push a west 0.7.2 and make that our minimum version.

I have tested this on Arch Linux with a fresh init + update + build + flash + debug + debugserver + attach + list. No issues observed.

Users who are running west 0.6 will see this error:

FATAL ERROR: Malformed manifest file .../nrf/west.yml (schema: .../west/manifest-schema.yml):
Schema validation failed:
 - Key 'version' was not defined. Path: ''.
 - Key 'import' was not defined. Path: '/projects/0'.

So heads-up that if you see this error, you need west 0.7.

I also tested our NCS extensions and made some changes to keep their output usable now that the way we will be doing mergeups depends on if the project in question is imported or not.

CI fails because it's using west 0.6. We should update the CI container once west 0.7.2 is available early next week for more testing.

@mbolivar mbolivar force-pushed the use-manifest-imports branch from c8a2673 to c745bbb Compare February 21, 2020 21:44
@mbolivar
Copy link
Author 

FATAL ERROR: Malformed manifest file /jenkins_workspace/workspace/latest_fw-nrfconnect-nrf_PR-1942/nrf/west.yml (schema: /usr/local/lib/python3.6/site-packages/west/manifest-schema.yml):
Schema validation failed:
 - Key 'version' was not defined. Path: ''.
 - Key 'import' was not defined. Path: '/projects/0'.

Yep, looks like CI is running 0.6.

@thst-nordic -- I'm going to cut west 0.7.2 on Monday. Can the CI container be updated to include that when it's ready, so we can test this with that version?

@mbolivar mbolivar force-pushed the use-manifest-imports branch 3 times, most recently from 23d51a1 to 6695dd0 Compare February 22, 2020 00:19
@mbolivar
Copy link
Author

@thst-nordic -- I'm going to cut west 0.7.2 on Monday. Can the CI container be updated to include that when it's ready, so we can test this with that version?

West 0.7.2 is out; ready for CI to use it to test this PR.

@mbolivar mbolivar requested a review from ru-fu as a code owner February 24, 2020 21:04
@mbolivar mbolivar force-pushed the use-manifest-imports branch 2 times, most recently from be8dc55 to f2ed593 Compare February 24, 2020 23:45
@ru-fu ru-fu added the doc-required PR must not be merged without tech writer approval. label Feb 25, 2020
@b-gent b-gent self-requested a review February 25, 2020 08:52
@mbolivar
Copy link
Author

@thst-nordic anything I can do to help get the CI container updated to use the new west?

@thst-nordic
Copy link
Contributor

I made a new docker image with zephyr sdk=0.11.1, and with west=0.7.3, it will be used on next CI run.

@thst-nordic thst-nordic force-pushed the use-manifest-imports branch 2 times, most recently from 7e200c8 to aa7a18d Compare February 26, 2020 15:42
@joerchan joerchan removed their request for review March 2, 2020 11:54
carlescufi
carlescufi approved these changes Mar 2, 2020
View reviewed changes
Copy link
Contributor

@carlescufi carlescufi left a comment

Choose a reason for hiding this comment

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

LGTM now, thanks @mbolivar

@mbolivar mbolivar force-pushed the use-manifest-imports branch 2 times, most recently from 89ed74a to b261057 Compare March 2, 2020 16:11
@mbolivar mbolivar requested a review from tejlmand March 2, 2020 16:17
@mbolivar
Copy link
Author

mbolivar commented Mar 2, 2020

@thst-nordic did I accidentally force push over something needed to test with west 0.7.2? I am seeing:

+ west init -l nrf

=== Initializing from existing manifest repository nrf

FATAL ERROR: Malformed manifest file /jenkins_workspace/workspace/latest_fw-nrfconnect-nrf_PR-1942@2/nrf/west.yml (schema: /usr/local/lib/python3.6/site-packages/west/manifest-schema.yml):

Schema validation failed:
 - Key 'version' was not defined. Path: ''.
 - Key 'import' was not defined. Path: '/projects/0'.
script returned exit code 1

I don't think so, because the log for f2ed593 doesn't have anything that looks CI-related, but thought I should check.

@tejlmand tejlmand mentioned this pull request Mar 3, 2020
Merged
@mbolivar mbolivar force-pushed the use-manifest-imports branch from b261057 to 768e351 Compare March 5, 2020 23:20
@mbolivar
Copy link
Author

mbolivar commented Mar 5, 2020

Looks like this is still blocked on a container update

@mbolivar mbolivar force-pushed the use-manifest-imports branch from 768e351 to 88e9003 Compare March 10, 2020 17:01
@ru-fu ru-fu requested review from umapraseeda and removed request for ru-fu March 17, 2020 09:44
@mbolivar mbolivar force-pushed the use-manifest-imports branch from 88e9003 to 93a1417 Compare March 19, 2020 18:13
@CLAassistant
Copy link

CLAassistant commented Mar 19, 2020 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@carlescufi
Copy link
Contributor

@mbolivar can you please rebase it?

@mbolivar mbolivar force-pushed the use-manifest-imports branch from 93a1417 to 342c4a4 Compare March 25, 2020 16:33
@mbolivar-nordic
Copy link
Contributor

@mbolivar can you please rebase it?

done

@mbolivar-nordic mbolivar-nordic self-requested a review March 25, 2020 16:50
@mbolivar-nordic
Copy link
Contributor

@thst-nordic thanks for unblocking this!

Here's what I see in the docs build:

+ utils/run_build.sh

Collecting pip==19.3.1

  Downloading https://files.pythonhosted.org/packages/00/b6/9cfa56b4081ad13874b0c6f96af8ce16cfbc1cb06bedf8e9164ce5551ec1/pip-19.3.1-py2.py3-none-any.whl (1.4MB)

Installing collected packages: pip

  Found existing installation: pip 18.1

    Uninstalling pip-18.1:

      Successfully uninstalled pip-18.1

Successfully installed pip-19.3.1

Ignoring windows-curses: markers 'sys_platform == "win32"' don't match your environment

ERROR: Double requirement given: west>=0.7.2 (from -r /jenkins_workspace/workspace/nrfconnect-nrf_doc_PR-NRF-1942_3/test_doc/../nrf/scripts/requirements.txt (line 5)) (already in west>=0.6.2 (from -r /jenkins_workspace/workspace/nrfconnect-nrf_doc_PR-NRF-1942_3/test_doc/../zephyr/scripts/requirements.txt (line 24)), name='west')

I can't find a run_build.sh in the NCS anywhere, so I'm not sure how to debug. Any advice? My guess is there's a pip install -r nrf/scripts/requirements.txt -r zephyr/scripts/requirements.txt somewhere that could be split into two pip install commands instead.

@carlescufi carlescufi force-pushed the use-manifest-imports branch 2 times, most recently from 3075fe6 to 227ee2e Compare March 26, 2020 12:50
@thst-nordic
Copy link
Contributor

this is ready to merge after west.yml conflict fixed.

@thst-nordic thst-nordic self-requested a review March 26, 2020 14:52
@carlescufi carlescufi force-pushed the use-manifest-imports branch from 227ee2e to 1f68051 Compare March 26, 2020 15:18
@mbolivar-nordic
west.yml: use zephyr imports
5616a4a 
Use the west 0.7 manifest import feature to pull in projects from
upstream Zephyr instead of maintaining them by hand.

Use a name whitelist to get started with, to keep things explicit.
This means the set of repositories from upstream has to be maintained
by hand, but their paths and revisions no longer do. Whenever the
zephyr "revision:" is updated, the paths and revisions of imported
projects are kept up to date automatically by west.

The NCS west extensions commands require west 0.7, but since that is
now the minimum version required by the manifest and requirements.txt,
there's no need to do special checking about that on our own here.

Make some other changes to these extensions. Some are generic, others
are specifically meant to deal with mergeups now that we're using
imports.

ncs-loot:

- Skip output for each project unless there are OOT patches.
  (This is disabled in verbose mode.)

- Only print the banner for a project after the loot has been
  calculated and we're ready to print it.

- Add a hint about output being limited when --file is used. This
  argument can cause the total list of files output to be smaller than
  the output list.

ncs-compare:

- Improve the output for downstream patches which are merged upstream,
  using singular correctly if there's just one, and numbering the
  possibilities if there's more than one.

- Use explicit paths on the user's computer when possible

- When an imported project has fallen behind, the way to update it is
  by doing the zephyr mergeup. That way, we get the updated revision for
  free.

  Track imported projects and let the user know that's all they have to
  do if that happens.

- Other miscellany

both:

- Print shorter --help lines. Control the indentation manually in
  descriptions.

- use "NCS" instead of "nrf"

Keep the development model document up to date, mainly with examples
for how to use manifest imports in workflow 4. Add some other changes
that follow from taking a look at this page in light of this new west
feature.

Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
@mbolivar mbolivar force-pushed the use-manifest-imports branch from 1f68051 to 5616a4a Compare March 26, 2020 15:24
@carlescufi carlescufi merged commit 68d743c into nrfconnect:master Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tejlmand tejlmand tejlmand approved these changes

@SebastianBoe SebastianBoe SebastianBoe approved these changes

@carlescufi carlescufi carlescufi approved these changes

@thst-nordic thst-nordic thst-nordic approved these changes

@umapraseeda umapraseeda umapraseeda approved these changes

@rlubos rlubos Awaiting requested review from rlubos

@b-gent b-gent Awaiting requested review from b-gent

@anangl anangl Awaiting requested review from anangl

@hakonfam hakonfam Awaiting requested review from hakonfam

@ioannisg ioannisg Awaiting requested review from ioannisg

@joakimtoe joakimtoe Awaiting requested review from joakimtoe

@jtguggedal jtguggedal Awaiting requested review from jtguggedal

@lemrey lemrey Awaiting requested review from lemrey

@nordic-krch nordic-krch Awaiting requested review from nordic-krch

@pdunaj pdunaj Awaiting requested review from pdunaj

@mbolivar-nordic mbolivar-nordic Awaiting requested review from mbolivar-nordic

Assignees

@carlescufi carlescufi

Labels
doc-required PR must not be merged without tech writer approval.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@mbolivar @thst-nordic @CLAassistant @carlescufi @mbolivar-nordic @SebastianBoe @tejlmand @umapraseeda @ru-fu