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

samples: doc: Added README for POSIX samples #67461

Merged
merged 2 commits into from
Jan 19, 2024

Conversation

dougwfoster
Copy link
Contributor

Added README.rst for eventfd and uname POSIX samples.
Helps fix #27805

@zephyrbot zephyrbot added area: POSIX POSIX API Library area: Samples Samples labels Jan 10, 2024
Copy link

Hello @dougwfoster, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

cfriedt
cfriedt previously approved these changes Jan 11, 2024
Copy link
Member

@cfriedt cfriedt left a comment

Choose a reason for hiding this comment

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

LGTM - a great follow up commit or PR would be to list POSIX samples in the os services / portability / POSIX doc area.

Would be happy to re-approve if you wish to add that here, @dougwfoster ?

samples/posix/eventfd/README.rst Outdated Show resolved Hide resolved
samples/posix/uname/README.rst Outdated Show resolved Hide resolved
@kartben kartben self-assigned this Jan 11, 2024
Copy link
Collaborator

@kartben kartben left a comment

Choose a reason for hiding this comment

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

Thanks!
Additional note: line wrapping is a bit inconsistent in the two proposed files. I realize this might be due to copy-pasting from the other POSIX sample README, but it would make sense to adjust to have consistent 100-character long lines.

samples/posix/eventfd/README.rst Show resolved Hide resolved
:board: qemu_x86
:goals: run
:compact:

Copy link
Collaborator

@kartben kartben Jan 11, 2024

Choose a reason for hiding this comment

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

do you want to add the "For comparison, to build directly for your host OS if it is POSIX compliant (for ex. Linux):" ... thing here too? It's actually quite a nice thing to mention.

Copy link
Contributor Author

@dougwfoster dougwfoster Jan 12, 2024

Choose a reason for hiding this comment

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

I noticed the uname sample does not have a Makefile.host file. I tried adding one but ran into build errors due to the include files of the sample. I decided not to add a Makefile.host for the uname sample

Copy link
Member

Choose a reason for hiding this comment

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

@dougwfoster this now can be done since #67648 is merged

samples/posix/uname/README.rst Outdated Show resolved Hide resolved
Overview
********

This sample application demonstrates using the POSIX eventfd()
Copy link
Collaborator

Choose a reason for hiding this comment

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

adding a reference to eventfd() doc would be welcome here, but not sure why there's none on opengroup webpage? @cfriedt might know

Copy link
Member

@cfriedt cfriedt Jan 11, 2024

Choose a reason for hiding this comment

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

eventfd() is not yet part of POSIX. It just happens to be lumped in with POSIX in Zephyr because it's available on most POSIX systems, is mostly dependent & compatible already on existing POSIX APIs, etc.

I'm ok taking eventfd() under the POSIX API wing in Zephyr, in any case.

samples/posix/eventfd/README.rst Show resolved Hide resolved
samples/posix/uname/README.rst Show resolved Hide resolved
samples/posix/uname/README.rst Outdated Show resolved Hide resolved
@dougwfoster
Copy link
Contributor Author

LGTM - a great follow up commit or PR would be to list POSIX samples in the os services / portability / POSIX doc area.

Would be happy to re-approve if you wish to add that here, @dougwfoster ?

That's a good idea. I can add a POSIX Sample section there or list them in the POSIX Applications in Zephyr section.

@cfriedt
Copy link
Member

cfriedt commented Jan 12, 2024

@dougwfoster - I think the samples portion should probably be automatically built (as long as the .rst files are captured by the main sample rst)

From the os services / portability / POSIX page, maybe just a link back to the samples would be good?

@kartben could probably be more specific.

kartben
kartben previously approved these changes Jan 16, 2024
Comment on lines 29 to 31
make -f samples/posix/eventfd/Makefile.host

The make output file will be located in samples/posix/eventfd.
Copy link
Collaborator

Choose a reason for hiding this comment

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

non blocking: kind of not ideal that this adds a new untracked file to the git repo, but I guess it was already like that before.

@dougwfoster dougwfoster requested a review from cfriedt January 16, 2024 20:06
samples/posix/eventfd/README.rst Outdated Show resolved Hide resolved
samples/posix/uname/README.rst Outdated Show resolved Hide resolved
samples/posix/eventfd/Makefile.host Outdated Show resolved Hide resolved
Added README.rst for eventfd and uname samples. Updated README for
gettimeofday to align with other READMEs.Updated Makefile.host
file for samples to store output file in 'build' directory.

Signed-off-by: Doug Foster <[email protected]>
Update index.rst located in doc/services/portability/posix/overview
to include link to POSIX samples.

Signed-off-by: Doug Foster <[email protected]>
Copy link
Member

@cfriedt cfriedt left a comment

Choose a reason for hiding this comment

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

LGTM - the only slight inaccuracy is that events is not POSIX (yet), even though it is available on many POSIX operating systems.

Otherwise, I think maybe @kartben could potentially offer some style suggestions?

@carlescufi carlescufi merged commit 6a1f48b into zephyrproject-rtos:main Jan 19, 2024
15 checks passed
Copy link

Hi @dougwfoster!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

@dougwfoster dougwfoster deleted the posix-sample-readme branch February 9, 2024 04:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: POSIX POSIX API Library area: Samples Samples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

samples without README
7 participants