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

Build application in sdk-nrf-toolchain docker image #15

Merged
merged 1 commit into from
May 7, 2024

Conversation

jangalda-nsc
Copy link
Contributor

@jangalda-nsc jangalda-nsc commented Apr 22, 2024

@gmarull regarding your comments to previous attempt:

1. Rework Zephyr build.yml instead of having 2 CI jobs - I believe it is better to keep 2 workflow files - using docker covers some cases, but customers may want to build their applications on Windows/Mac nodes or use some tools which are not available in ncs docker image.

2. Remove any hacks from the job (e.g. apt installs which can be part of the Docker image) - done

3. Use latest Docker image on main, only tag on releases. - we do not release latest tag for sdk-nrf-toolchain image at this moment. If such feature is added, I can change workflow file

Format commits as in NCS (nrf noup in this case) - done (I hope)

@jangalda-nsc jangalda-nsc force-pushed the build-in-ncs-docker-3 branch from db40650 to 35c3518 Compare April 22, 2024 13:39
@jangalda-nsc jangalda-nsc marked this pull request as draft April 22, 2024 13:39
Copy link
Member

@gmarull gmarull left a comment

Choose a reason for hiding this comment

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

lgtm, but this should be brought as a noup commit, and it should replace the existing job (we will only build using NCS docker image here)

@jangalda-nsc
Copy link
Contributor Author

@gmarull PR is in the draft mode - I'm testing fixes in docker image and how they behave with GH action limitations. I'll make requested changes and convert to regular PR when it's ready for review.
Unfortunately, I can't temporary remove you from reviewers list since you are codeowners, so sorry for all spam messages with updates

@jangalda-nsc jangalda-nsc force-pushed the build-in-ncs-docker-3 branch 11 times, most recently from 5718037 to e8aeb7a Compare April 25, 2024 12:21
@jangalda-nsc jangalda-nsc marked this pull request as ready for review April 25, 2024 12:29
Copy link
Member

@gmarull gmarull left a comment

Choose a reason for hiding this comment

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

upstream workflow should be deleted as part of this commit (ncs specific workflow will replace the zephyr one)

@gmarull
Copy link
Member

gmarull commented Apr 25, 2024

@gmarull regarding your comments to previous attempt:

1. Rework Zephyr build.yml instead of having 2 CI jobs - I believe it is better to keep 2 workflow files - using docker covers some cases, but customers may want to build their applications on Windows/Mac nodes or use some tools which are not available in ncs docker image.

The windows/mac builds are really GH windows/mac runners using the Zephyr SDK, so we test for free those environments using the recommended Zephyr setup. But since we're in NCS context, we should only build on what we officially support. So if we want to run on windows/mac then fine, but using the procedures we officially recommend. To me, it will be confusing for customers to understand which workflow to pick, as both do the same but, to build applications that use certain NCS features only NCS workflow will work fine. Now both work because ncs-example-application does not use any NCS feature that may require extra packages from NCS requirements.

@gmarull gmarull force-pushed the main branch 3 times, most recently from 9d60f65 to 0a178b2 Compare May 2, 2024 09:16
Add an action which builds and tests application using ncs-docker image.
@jangalda-nsc jangalda-nsc force-pushed the build-in-ncs-docker-3 branch from e8aeb7a to 7eaadcf Compare May 6, 2024 13:54
@jangalda-nsc
Copy link
Contributor Author

jangalda-nsc commented May 6, 2024

@gmarull I removed build.xml file to avoid confusion and resolved conflicts. I think, from maintenance point of view it will be easier to remove upstream workflow instead of replacing it. If there are any changes in upstream workflow, it will be more obvious that we should ignore them

@jangalda-nsc jangalda-nsc requested a review from gmarull May 7, 2024 06:40
@gmarull gmarull merged commit fbed039 into nrfconnect:main May 7, 2024
3 checks passed
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