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.

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

manifest: move optional modules to a submanifest and make them optional #61505

Merged
merged 8 commits into from
Oct 1, 2023

Conversation

nashif
Copy link
Member

@nashif nashif commented Aug 15, 2023

Move optional modules to a submanifest and make them optional by
default.
The idea is to look
at the optional manifest as an area for modules that work with Zephyr,
but not needed directly by zephyr. This could be documented somewhere
for discovery purposes allowing users to enable such modules in their
downstream if desired.

See #54276

Signed-off-by: Anas Nashif anas.nashif@intel.com

@zephyrbot
Copy link
Collaborator

zephyrbot commented Aug 15, 2023

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff
sof zephyrproject-rtos/sof@e32b825 zephyrproject-rtos/sof@ee40f61 zephyrproject-rtos/sof@e32b825c..ee40f61b

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@nashif
Copy link
Member Author

nashif commented Aug 15, 2023

as discussed in babblesim related changes some time ago, we will need some CI integration in place to support something like that and some clarification on use of submanifests etc.

@nashif
Copy link
Member Author

nashif commented Aug 16, 2023

$ west init && du -sh
873M
$ west update && du -sh
4.5G
$ remove exclusion of optional group...
4.7G

so, our issue is not the optional modules I guess :)

HALs:

gale:hal: du -s * | sort -n
804	wurthelektronik
960	intel
1096	quicklogic
1568	altera
1684	libmetal
1980	renesas
2076	telink
2388	ethos_u
2976	xtensa
7832	openisa
12264	microchip
14012	rpi_pico
16316	gigadevice
16840	nuvoton
19104	cmsis
20196	ambiq
33708	ti
63536	st
73772	nordic
89244	atmel
98076	infineon
196376	silabs
384120	espressif
679252	stm32
838652	nxp

libs:

gale:lib: du -s * | sort -n
1768	liblc3
2756	canopennode
3172	zcbor
3644	open-amp
5672	uoscore-uedhoc
7560	lz4
8732	zscilib
9944	nanopb
17032	chre
67488	loramac-node
135724	openthread
186172	gui
196740	picolibc
217420	acpica

.....

@fabiobaltieri
Copy link
Member

$ west init && du -hs
876M    .
$ west update -o=--depth=1 -n && du -sh
3.7G    .

Maybe we could suggest that in documentation as a start, I'd imagine most users don't care about modules commit history.

@fabiobaltieri
Copy link
Member

> 217420	acpica

Funny, this one is 8MB of git objects with a shallow fetch, a full history fetch is 163MB (of git objects) in commit history since 2005. Maybe we should consider that for new modules, doubt that much history is useful here.

@nashif
Copy link
Member Author

nashif commented Aug 16, 2023

isnt this the same as
west config --global update.narrow true

@nashif
Copy link
Member Author

nashif commented Aug 16, 2023

snt this the same as
west config --global update.narrow true

no I guess

@fabiobaltieri
Copy link
Member

fabiobaltieri commented Aug 16, 2023

snt this the same as
west config --global update.narrow true

no I guess

Right, I have that in that command line (-n) don't think it does much in this case but if you use it on application where the main zephyr repo is a module it skips the tags, which saves a lot. It does something else as well but I can't quite grasp what right now for some reason It also prevents fetching objects from all remote branches: https://github.com/zephyrproject-rtos/west/blob/9e8f5002f2ee86b7150c039cb69aa653ff454162/src/west/app/project.py#L1410

@nashif
Copy link
Member Author

nashif commented Aug 16, 2023

there is a clone-depth option, but that is per module AFAIK, not something to set globaly for all update ops

@fabiobaltieri
Copy link
Member

fabiobaltieri commented Aug 16, 2023

there is a clone-depth option, but that is per module AFAIK, not something to set globaly for all update ops

Yeah, also it does not apply to submodules imported modules, if you do something like: https://github.com/zmkfirmware/zmk/blob/c957348e6187ffde3c073e8c367a60a7ee16b12c/app/west.yml#L7-L32 the modules still have full history.

@fabiobaltieri
Copy link
Member

zmk-a$ time west update
real    2m18.010s
zmk-a$ du -hs
2.5G    .
$ du -hs zephyr/.git
283M    zephyr/.git
zmk-b$ time west update -o=--depth=1 -n
real    1m15.526s
zmk-b$ du -hs
1.8G    .
$ du -hs zephyr/.git
57M     zephyr/.git

Both with shallow main repository, the difference is just in module objects and the tag objects for the main repo.

Copy link
Collaborator

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

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

I like it and this is essentially something I've always been doing when cloning anyhow so means one less step for me. But with this, is there a way to enable just one of the optional modules, or can you only enable all of them. E.g. if someone wants to get zephyr and just enable nanopb and thrift, can they do that with west commands? And also do that in their own west manifest which imports the zephyr one?

@nashif
Copy link
Member Author

nashif commented Aug 17, 2023

I like it and this is essentially something I've always been doing when cloning anyhow so means one less step for me. But with this, is there a way to enable just one of the optional modules, or can you only enable all of them. E.g. if someone wants to get zephyr and just enable nanopb and thrift, can they do that with west commands? And also do that in their own west manifest which imports the zephyr one?

that is the same question asked here: #54276 (comment)

AFAIK, the short answer is no. This is where we get into 'package management'. Optional manifest will serve as a list of modules you can enable in your workspace if you are doing something localy, i.e. in your own manifest.

You can always modify the upstream manifest locally and pull whatever you want, but what would be the usecase here if those are optional and nothing in the zephyr tree depend on them?

Maybe you want to add a new subsystem that usess one of those, say you add a new subsystem that uses lz4 compression. So, in this case, you put it in the main manifest, develop your subsystem and submit this to zephyr moving lz4 from optional to something used in tree.

If you are working on something out of the tree, then you add it to your own manifest and will remain optional upstream...

@nordicjm
Copy link
Collaborator

nordicjm commented Aug 17, 2023

Ideally it is so an external manifest could enable it and get the correct version for that commit of zephyr, i.e. similar to importing with an allow/block list. So someone might want zephyr and thrift, in their manifest they pull in zephyr (and might get multiple versions, i.e. support multiple branches) and want to pull in whatever version of thrift is in the zephyr optional manifest for that version, otherwise they then have to maintain their own lists for each zephyr version and check the commits against the zephyr optional manifest file.

Actually thinking about it, I assume that can already be done by the external manifest enabling the group filter and using an allow/block list to get only the optional modules they want - tested and it does work, all good!

Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

This is just a POC, might require some more thought.

Still true? Looks much better than a POC to me now...

*****

This extension introduces a new directive: ``manifest-projects-table``. It can
be used in the code as::
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
be used in the code as::
be used in sphinx source as::


- ``manifest_projects_table_manifest``: Path to the manifest file.

Copyright (c) Nordic Semiconductor ASA 2022
Copy link
Collaborator

Choose a reason for hiding this comment

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

To keep divergence somewhat under control can you please add the "upstream" URL where this came from?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no intention to keep this compatible or in sync with "upstream"

@nashif nashif removed the TSC Topics that need TSC discussion label Sep 27, 2023
jhedberg
jhedberg previously approved these changes Sep 27, 2023
Comment on lines +142 to +146
Similar to optional modules, but added to the Zephyr project as an entry in the
documentation using a pre-defined template. This type of modules exists outside the
Zephyr project manifest with documentation instructing users and developers how
to integrate the functionality.

Copy link
Member

@cfriedt cfriedt Sep 29, 2023

Choose a reason for hiding this comment

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

Suggested change
Similar to optional modules, but added to the Zephyr project as an entry in the
documentation using a pre-defined template. This type of modules exists outside the
Zephyr project manifest with documentation instructing users and developers how
to integrate the functionality.
Similar to optional modules but added to the Zephyr project as an entry in the
documentation using a pre-defined template. These type of modules exists outside the
Zephyr project manifest. External modules are responsible for providing
documentation instructing users and developers how the module may be used with
Zephyr.

Move optional modules to a submanifest and make them optional by
default.
This is just a POC, might require some more thought. The idea is to look
at the optional manifest as an area for modules that work with Zephyr,
but not needed directly by zephyr. This could be documented somewhere
for discovery purposes allowing users to enable such modules in their
downstream if desired.

See zephyrproject-rtos#54276

Signed-off-by: Anas Nashif <anas.nashif@intel.com>
Document integration modes of modules.

Signed-off-by: Anas Nashif <anas.nashif@intel.com>
If a module is not available, then it is optional, so do not error in
--integration mode.

Signed-off-by: Anas Nashif <anas.nashif@intel.com>
While we cleanup, pull optional modules as before to keep CI happy.

Signed-off-by: Anas Nashif <anas.nashif@intel.com>
Generate index of west projects and information about them.
Based on an extension from sdk-nrf repo.

Signed-off-by: Anas Nashif <anas.nashif@intel.com>
This sample depends on lz4.

Signed-off-by: Anas Nashif <anas.nashif@intel.com>
The sample depends on on the tflite-micro module.

Signed-off-by: Anas Nashif <anas.nashif@intel.com>
Those samples/tests depend on psa-arch-tests module.

Signed-off-by: Anas Nashif <anas.nashif@intel.com>
@nashif nashif added this to the v3.5.0 milestone Sep 29, 2023
Comment on lines +5 to +6
This extension allows to render a table containing the revisions of the projects
present in a manifest file.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This extension allows to render a table containing the revisions of the projects
present in a manifest file.
This extension renders a table containing the revisions of the projects
present in a manifest file.

###################

See :ref:`external-contributions` for more information about
this contributing and review process for imported components.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
this contributing and review process for imported components.
the contributing and review process for imported components.

Copy link
Collaborator

@hakehuang hakehuang left a comment

Choose a reason for hiding this comment

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

test OK in all NXP paltform

@jhedberg jhedberg merged commit a3eff88 into zephyrproject-rtos:main Oct 1, 2023
34 checks passed
pdgendt referenced this pull request Oct 15, 2023
Introduce a helper function zephyr_nanopb_sources to generate
source files and add these to a target.

Signed-off-by: Pieter De Gendt <pieter.degendt@basalte.be>
@soburi soburi mentioned this pull request Oct 17, 2023
@marc-hb
Copy link
Collaborator

marc-hb commented Dec 30, 2023

$ west update && du -sh
4.5G
$ remove exclusion of optional group...
4.7G
so, our issue is not the optional modules I guess :)

I just discovered by "chance" that there is 1 Gigabyte-big "SDK dump" in hal_nxp:

EDIT: I haven't looked at it and I won't but from past experience I would expect a lot of "copyright fun" there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.