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

posix: implement uname #59981

Merged
merged 3 commits into from
Jul 9, 2023
Merged

Conversation

ycsin
Copy link
Member

@ycsin ycsin commented Jul 4, 2023

Implementation for posix api: uname

*** Booting Zephyr OS build zephyr-v3.4.0-633-gc38dd60da451 ***

Printing everything in utsname...
sysname[7]: Zephyr
nodename[7]: zephyr
release[9]: 3.4.99
version[61]: zephyr-v3.4.0-633-gc38dd60da451 Jul  6 2023 10:29:50
machine[8]: riscv64


uart:~$ uname -a
Zephyr zephyr 3.4.99 zephyr-v3.4.0-633-gc38dd60da451 Jul  6 2023 10:29:50 riscv64 qemu_riscv64

MacOS as a reference:

(.venv) ycsin-mba:zephyrproject ycsin$ uname -a
Darwin ycsin-mba 22.5.0 Darwin Kernel Version 22.5.0: Mon Apr 24 20:53:44 PDT 2023; root:xnu-8796.121.2~5/RELEASE_ARM64_T8103 arm64

fixes #59924

  • update doc/services/portability/posix.rst
  • add to the testsuite
  • size_t

@ycsin ycsin requested review from nashif and cfriedt as code owners July 4, 2023 07:38
@zephyrbot zephyrbot added the area: POSIX POSIX API Library label Jul 4, 2023
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 - just a couple of nits and suggestions.

The sample is good, but it would also be good to add to the testsuite.

One tricky area in particular is the version string.

We actually do want to be fairly consistent here with e.g. the boot banner.

So it would make sense to detect and modify the string such that we can display

  • a tagged release (e.g. v3.5.0)
  • something commit-ish (IIRC git short ref plus epoch time? I can't remember)
  • whether the build is dirty (Linux uses a +, not sure how Zephyr does it)

Of possible, can you please also update doc/services/portability/posix.rst?

Great work! Thanks @ycsin 🚀🪁👍🍍

include/zephyr/posix/sys/utsname.h Outdated Show resolved Hide resolved
include/zephyr/posix/sys/utsname.h Show resolved Hide resolved
lib/posix/utsname.c Outdated Show resolved Hide resolved
lib/posix/utsname.c Outdated Show resolved Hide resolved
@cfriedt
Copy link
Member

cfriedt commented Jul 4, 2023

Some compliance fixes..

@cfriedt
Copy link
Member

cfriedt commented Jul 4, 2023

IIRC, we cannot yet build for native_posix. I would suggest filtering CONFIG_ARCH_POSIX in your sample

@ycsin ycsin force-pushed the pr/posix-uname branch 5 times, most recently from 58c7e3e to ed10e85 Compare July 4, 2023 13:15
@ycsin
Copy link
Member Author

ycsin commented Jul 4, 2023

We actually do want to be fairly consistent here with e.g. the boot banner.

The version string is the same as the boot banner (I just copied & renamed)

  • a tagged release (e.g. v3.5.0)
  • something commit-ish (IIRC git short ref plus epoch time? I can't remember)

Should be covered by:

3.4.99 zephyr-v3.4.0-632-gfed4ab1928ba Jul  4 2023 19:01:24

Not epoch time though..

whether the build is dirty (Linux uses a +, not sure how Zephyr does it)

Looks like something to be done in the build system, not sure if @tejlmand has any idea?
I vaguely rmb that we used to have this somewhere..?

@ycsin ycsin force-pushed the pr/posix-uname branch 2 times, most recently from 80b4cbd to ee95091 Compare July 4, 2023 16:23
@zephyrbot zephyrbot added area: CMSIS API Layer CMSIS-RTOS API Layer area: Portability Standard compliant code, toolchain abstraction labels Jul 4, 2023
lib/posix/utsname.c Outdated Show resolved Hide resolved
@ycsin ycsin requested a review from cfriedt July 4, 2023 16:59
@ycsin ycsin force-pushed the pr/posix-uname branch 2 times, most recently from c5df7b4 to 87237ae Compare July 5, 2023 08:11
@tejlmand
Copy link
Collaborator

tejlmand commented Jul 5, 2023

whether the build is dirty (Linux uses a +, not sure how Zephyr does it)

Looks like something to be done in the build system, not sure if @tejlmand has any idea? I vaguely rmb that we used to have this somewhere..?

Adding -dirty (or anything indicating a dirty build) has been rejected in the past, ref: #13608 (review)

But the use-case / reason for not having -dirty can be handled manually by those who has special needs, as described here:
#39503 (comment)

As people has various ways of working / verifying patches, then I think a wider audience should be invited to discuss if we want to have -dirty (or similar) appended to short sha.
The implementation itself is very trivial, so just a matter of making a decision.

lib/posix/utsname.c Outdated Show resolved Hide resolved
cfriedt
cfriedt previously approved these changes Jul 5, 2023
Copy link
Collaborator

@keith-packard keith-packard left a comment

Choose a reason for hiding this comment

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

the strncpy usage needs to be fixed, otherwise my comments are just suggestions.

char sysname[sizeof("Zephyr")];
char nodename[CONFIG_POSIX_UNAME_NODENAME_LEN];
char release[sizeof("99.99.99")];
char version[CONFIG_POSIX_UNAME_VERSION_LEN];
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the size of this field can also be computed -- just need to insert the computation of the version string into this header. One less config value to deal with seems like a nice thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can actually compute everything using preprocessor defines here, at the expense of including 2 more headers & some macro magic then looks a bit complicated

Copy link
Member

@cfriedt cfriedt Jul 9, 2023

Choose a reason for hiding this comment

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

The version field can be fixed size as well, technically, too, if we omit the date and time, and it's compatible with the banner version as well.

char version[sizeof("zephyr-v99.99.99-9999-g0123456789ab")];

Ideally we could just include <version.h>, which is generated at build time and use the exact #defines that appear there.

I'm hesitant to do that, just because it really isn't referenced outside of banner.c, which gives me the impression that it's private somehow.

If that makes life easier though, by all means, just

struct utsname {
	char sysname[sizeof("Zephyr")];
	char nodename[CONFIG_POSIX_UNAME_NODENAME_LEN + 1];
	char release[sizeof(KERNEL_VERSION_STRING)];
	char version[sizeof(STRINGIFY(BUILD_VERSION))];
	char machine[sizeof(CONFIG_ARCH)];
};

And honestly, the __DATE__ and __TIME__ macros are probably not super critical.

I would be less hesitant to include <version.h> if it were namespaced properly (e.g. <zephyr/kernel/build_version.h>).

At the same time, there is plenty of time to improve things before v3.5.0, so no need to stall here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

__DATE__ and __TIME__ break build reproducibility and should not be enabled by default (not sure what is the current situation)

lib/posix/Kconfig Outdated Show resolved Hide resolved
lib/posix/uname.c Show resolved Hide resolved
@ycsin
Copy link
Member Author

ycsin commented Jul 6, 2023

Adding -dirty (or anything indicating a dirty build) has been rejected in the past

how about having a separate define to indicate this in "version.h", rather than appending to the hash directly?

@tejlmand
Copy link
Collaborator

tejlmand commented Jul 6, 2023

how about having a separate define to indicate this in "version.h", rather than appending to the hash directly?

Did you read the links ?
Especially: #13608 (review) .

Even if placing this info in a separate define to use in version.h then it will impact the hex file when the repo goes from clean --> dirty (or other way around).

So if this is to be changed, then please open a dedicated issue for discussing, and not in this PR.

Add implementation for posix uname.

Signed-off-by: Yong Cong Sin <ycsin@meta.com>
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.

One minor change request

samples/posix/uname/src/main.c Outdated Show resolved Hide resolved
@cfriedt cfriedt assigned ycsin and unassigned cfriedt Jul 7, 2023
@cfriedt
Copy link
Member

cfriedt commented Jul 7, 2023

So if this is to be changed, then please open a dedicated issue for discussing, and not in this PR.

@tejlmand - I believe @ycsin's changes have addressed your concerns, and with a couple of small changes, I'm ready to approve. Can you take a look again to ensure that your concerns were addressed?

ycsin added 2 commits July 7, 2023 23:07
Add a simple, testable sample to print everything in the utsname struct,
and added a `uname` shell cmd basically copied from nuttx just for fun.

Signed-off-by: Yong Cong Sin <ycsin@meta.com>
add ztest for uname api

Signed-off-by: Yong Cong Sin <ycsin@meta.com>
@cfriedt
Copy link
Member

cfriedt commented Jul 9, 2023

@keith-packard - does this one look OK to you now?

@cfriedt
Copy link
Member

cfriedt commented Jul 9, 2023

@ycsin - I'm OK getting 1 more approval for this and merging if you wouldn't mind a follow-up PR for namespacing <version.h> as <zephyr/kernel/banner_version.h> or something along those lines.

Copy link
Collaborator

@keith-packard keith-packard left a comment

Choose a reason for hiding this comment

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

Yeah, given the concerns about mixing too many bits into the application namespace, having compile-time checks that make sure the sizes are big enough seems good enough here.

@cfriedt cfriedt merged commit 0736448 into zephyrproject-rtos:main Jul 9, 2023
21 checks passed
@ycsin ycsin deleted the pr/posix-uname branch July 10, 2023 02:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: CMSIS API Layer CMSIS-RTOS API Layer area: Portability Standard compliant code, toolchain abstraction area: POSIX POSIX API Library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

posix: implement uname()
6 participants