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

include whole project in docker container #11

Merged
merged 1 commit into from
Mar 12, 2022

Conversation

jdoubleu
Copy link

This is a followup of #10.

Instead of only mounting the path from the input in the container, this will mount the whole repository under /app inside the container and then cd inside the container before building it.

This fixes the issue of repos which contain multiple stand-alone projects, which rely on code outside the project itself, respectively. Usually to be found in projects which also maintain examples in subfolders. What if esp32-s2-hmi-devkit-1/examples/smart-panel (from the README) relies on code e.g. in esp32-s2-hmi-devkit-1/driver/....

Copy link
Collaborator

@kumekay kumekay left a comment

Choose a reason for hiding this comment

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

@jdoubleu Thank you for opening this PR,
I suggested a change,

Please, also note that main branch has changed

action.yml Outdated Show resolved Hide resolved
@jdoubleu jdoubleu force-pushed the input-esp-version-adjustment branch from e9fc8c6 to fd61910 Compare March 9, 2022 10:15
@jdoubleu
Copy link
Author

jdoubleu commented Mar 9, 2022

Thanks for the feedback! I've just pushed my new changes.

I'm sticking with your 2nd suggestions: using the container workspace parameter. This would call idf.py build inside the subdirectory, rather than the root directory. This might have different implications regarding the environment. Imagine a custom build/CMakeLists.txt where it accesses, for whatever reason, a file in the current working dir (pwd). I assume this wouldn't work as expected when called from the root directory. Again this is probably an unlikely edge-case, I'm just trying to anticipate any future issues.

@kumekay
Copy link
Collaborator

kumekay commented Mar 12, 2022

@jdoubleu Thank you for the update, LGTM,
I also tested it for the case you described and it works fine https://github.com/kumekay/test_idf_project/runs/5522949237?check_suite_focus=true

@kumekay kumekay merged commit 0ab4167 into espressif:main Mar 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants