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

mbedtls: remove unused configuration header files #78641

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

mbedtls: remove unused configuration header files #78641

wants to merge 2 commits into from
+3 −984

Conversation

valeriosetti
Copy link
Collaborator

@valeriosetti valeriosetti commented Sep 18, 2024 •
edited
Loading

  • Remove all unused configuration header files. These were likely copied from Mbed TLS in the past, but not updated afterwards. At the same time they are not used in the Zephyr's scope, so it's useless to keep them around.

  • Do a bit of renaming:

    • config-tls-generic.h -> config-mbedtls.h
    • Kconfig.tls-generic -> Kconfig.mbedtls

@valeriosetti valeriosetti force-pushed the remove-unused-headers branch 3 times, most recently from ae350ba to b1ab664 Compare September 20, 2024 14:17
tomi-font
tomi-font requested changes Oct 11, 2024
View reviewed changes
Copy link
Collaborator

@tomi-font tomi-font left a comment

Choose a reason for hiding this comment

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

A very welcome rework!

@github-actions GitHub Actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Dec 11, 2024
@github-actions github-actions bot closed this Dec 26, 2024
@valeriosetti valeriosetti reopened this Jan 10, 2025
@valeriosetti valeriosetti force-pushed the remove-unused-headers branch 3 times, most recently from 390b173 to c82fd28 Compare January 10, 2025 15:32
@valeriosetti valeriosetti force-pushed the remove-unused-headers branch from c82fd28 to 6635aac Compare January 27, 2025 11:01
@valeriosetti valeriosetti marked this pull request as ready for review January 27, 2025 14:59
@zephyrbot zephyrbot requested a review from ithinuel February 17, 2025 10:59
@valeriosetti
Copy link
Collaborator Author

valeriosetti commented Feb 17, 2025 •
edited
Loading

Since it was a bit hard to get reviews on this, I strongly reduced the scope of the PR. Now it just removes unused files and renames what's left for Mbed TLS.
Other commits that were removed from this PR will be added to a follow up PR whenever this one will be merged.

valeriosetti added a commit to valeriosetti/mcuboot that referenced this pull request Feb 17, 2025
@valeriosetti
zephyr: update main Mbed TLS configuration header file
81388b3 
This PR should be merged in sync with zephyrproject-rtos/zephyr#78641.

Signed-off-by: Valerio Setti <[email protected]>
@valeriosetti valeriosetti mentioned this pull request Feb 17, 2025
Closed
@zephyrbot
Copy link
Collaborator

zephyrbot commented Feb 17, 2025 •
edited
Loading

The following west manifest projects have changed revision in this Pull Request:

Name Old Revision New Revision Diff

✅ All manifest checks OK

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

@zephyrbot zephyrbot added manifest manifest-mcuboot DNM (manifest) This PR should not be merged (controlled by action-manifest) labels Feb 17, 2025
@valeriosetti valeriosetti changed the title mbedtls: improve Kconfigs and configuration header files mbedtls: remove unused configuration header files Feb 18, 2025
de-nordic
de-nordic reviewed Feb 27, 2025
View reviewed changes
west.yml Outdated
@@ -303,7 +303,7 @@ manifest:
groups:
- crypto
- name: mcuboot
revision: 346f7374ff4467e40b5594658f8ac67a5e9813c9
revision: pull/124/head
Copy link
Collaborator

Choose a reason for hiding this comment

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

This has been closed, shouldn't this PR reference the mcutools/mcuboot PR?

Copy link
Collaborator Author

@valeriosetti valeriosetti Feb 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, the reference to zephyrproject-rtos/mcuboot#124 is surely not correct now, but I don´t know if I can directly point to mcu-tools/mcuboot#2222. While creating PR 2222 I noticed that the main branch of https://github.com/mcu-tools/mcuboot is a bit newer than https://github.com/zephyrproject-rtos/mcuboot, so I guess that the process is:

Am I wrong?

Copy link
Collaborator

Choose a reason for hiding this comment

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

* create a PR in https://github.com/zephyrproject-rtos/mcuboot which cherry-picks my fix

No.

Am I wrong?

Yes ;)

We sync mcuboot to have exactly the same history as upstream, so that is only done via full main branch syncs.
Like here:
#85671

@valeriosetti valeriosetti force-pushed the remove-unused-headers branch from 284e9d8 to 20bfe17 Compare March 11, 2025 15:40
@zephyrbot zephyrbot removed manifest manifest-mcuboot DNM (manifest) This PR should not be merged (controlled by action-manifest) labels Mar 11, 2025
tomi-font
tomi-font previously approved these changes Mar 12, 2025
View reviewed changes
str4t0m
str4t0m previously approved these changes Mar 12, 2025
View reviewed changes
@tomi-font
Copy link
Collaborator

@d3zd3z @ceolin please review

wearyzen
wearyzen previously approved these changes Mar 13, 2025
View reviewed changes
ceolin
ceolin previously approved these changes Mar 14, 2025
View reviewed changes
@kartben
Copy link
Collaborator

kartben commented Mar 18, 2025

@valeriosetti this will need rebasing - thx!

@valeriosetti
mbedtls: remove unnecessary headers
6739844 
These headers are basically a copy of those found in the official
Mbed TLS repo in the "configs" folder. However these are unmaintaned
(i.e. not updated when Mbed TLS is bumped to a new version) and
also unused in Zephyr. Therefore there's no benefit in keeping
them around.

Signed-off-by: Valerio Setti <[email protected]>
@valeriosetti
mbedtls: rename tls-generic Kconfig and configuration header file
17ee331 
Since there's now a single Kconfig and header file for Mbed TLS it makes
more sense to name them:
- Kconfig.mbedtls (instead of Kconfig.tls-generic)
- config-mbedtls.h (instead config-tls-generic.h)

Signed-off-by: Valerio Setti <[email protected]>
@valeriosetti valeriosetti dismissed stale reviews from ceolin, wearyzen, str4t0m, and tomi-font via 17ee331 March 18, 2025 15:52
@valeriosetti valeriosetti force-pushed the remove-unused-headers branch from 20bfe17 to 17ee331 Compare March 18, 2025 15:52
@valeriosetti
Copy link
Collaborator Author

Rebase done without any code change. The PR is ready for reviews again.

dkalowsk
dkalowsk approved these changes Mar 20, 2025
View reviewed changes
Copy link
Contributor

@dkalowsk dkalowsk left a comment

Choose a reason for hiding this comment

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

Thanks for this clean up and making it easier for end users.

@valeriosetti
Copy link
Collaborator Author

@valeriosetti this will need rebasing - thx!

@kartben rebase done, CI green and already got some approvals. Can you please merge this before any conflict pops up?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dkalowsk dkalowsk dkalowsk approved these changes

@de-nordic de-nordic de-nordic approved these changes

@tomi-font tomi-font tomi-font approved these changes

@d3zd3z d3zd3z Awaiting requested review from d3zd3z

@ithinuel ithinuel Awaiting requested review from ithinuel

@wearyzen wearyzen Awaiting requested review from wearyzen

@str4t0m str4t0m Awaiting requested review from str4t0m

@ceolin ceolin Awaiting requested review from ceolin

Assignees

@d3zd3z d3zd3z

@ceolin ceolin

Labels
area: mbedTLS / PSA Crypto
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@valeriosetti @zephyrbot @tomi-font @kartben @ceolin @dkalowsk @str4t0m @de-nordic @wearyzen @d3zd3z