[devcontainer] Version bump and enhanced UX. #35403
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Review changes with SemanticDiff. Analyzed 1 of 4 files.
|
pullapprove
bot
requested review from
andy31415,
andyg-apple,
anush-apple,
arkq,
axelnxp,
bauerschwan,
bzbarsky-apple,
carol-apple,
cecille,
chapongatien,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
doru91,
fessehaeve,
harimau-qirex,
harsha-rajendran,
hawk248 and
hicklin
pullapprove
bot
requested review from
tima-q,
tobiasgraf,
turon,
vivien-apple,
wiba-nordic,
woody-apple,
younghak-hwang and
yufengwangca
andy31415
reviewed
Sep 4, 2024
andy31415
reviewed
Sep 4, 2024
andy31415
requested changes
Sep 4, 2024
There was a problem hiding this comment.
This probably should be split between devcontainer changtes and dockerfile changes.
I have reservations about the dockerfile changes - it makes it more towards a generic full fledged development setup, however I believe that we may want to figure out some different paths for that as everyone may have their own development environment and scripts.
|
PR #35403: Size comparison from 44f8837 to 9233380 Full report (12 builds for nrfconnect, nxp, qpg, stm32, tizen)
|
|
PR #35403: Size comparison from 44f8837 to 9fb7f77 Full report (77 builds for bl602, bl702, bl702l, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
arkq
reviewed
Sep 4, 2024
arkq
reviewed
Sep 4, 2024
|
PR #35403: Size comparison from 44f8837 to 9938e43 Full report (77 builds for bl602, bl702, bl702l, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #35403: Size comparison from 44f8837 to 51a8eb1 Full report (77 builds for bl602, bl702, bl702l, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
andy31415
approved these changes
Sep 6, 2024
andy31415
reviewed
Sep 6, 2024
woody-apple
approved these changes
Sep 7, 2024
yyzhong-g
pushed a commit
to yyzhong-g/connectedhomeip
that referenced
this pull request
Dec 12, 2024
* Version bump, remove unnecessary libs * Allow environment location modification * Add apt installs to base image * Speed-up image building * Remove unneeded apt installs * Expose and document important env variable * Add all pigweed environments to .gitignore * Revert and reformat comment * Compatibility fix
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue
The current devcontainer image version is 22, while the latest available version is 74. Upgrading to the latest version will resolve several compatibility issues.
The directory of bootstrap files is hardcoded in the base image but it should be visibly configurable.
The devcontainer build process is inefficient, with a significant amount of time spent on recursively
chowning unnecessary files.Proposed Solution
Update the
devcontainer.jsonto use the latest image version. Additionally, remove some unnecessary dependencies inDockerfile.Move the declaration of
PW_ENVIRONMENT_ROOTenvironment variable from base image todevcontainer.json.Eliminate certain
chownoperations, asgit config --global --add safe.directory "*"with other minor changes provide a faster and sufficient alternative.Extra notes
apt installs from the
.devcontainer/Dockerfileshould be moved to the base image. This PR adds these apt installs to the base image and flags the ones in.devcontainer/Dockerfileto be removed in the next version when this gets merged.Testing
Successfully compiled a subset of examples for the affected build targets.