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

generate errno definitions from CSV #15579

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

generate errno definitions from CSV #15579

wants to merge 4 commits into from

Conversation

yamt
Copy link
Contributor

@yamt yamt commented Jan 16, 2025

Summary

generate errno definitions from a CSV file for flexibility.

i plan to use this CSV file for other things like host <-> nuttx errno conversion logic.

Impact

Testing

  • i investigated the diff between the generated file and the original errno.h
  • bulid-tested sim:toywasm

this is a machine-friendly version of errno.h

using this file, i plan to generate:

* the corresponding C definitions in errno.h.

* strerror tables.

* host <-> nuttx errno conversion logic for sim.
@github-actions github-actions bot added Area: Tooling Area: OS Components OS Components issues Size: L The size of the change in this PR is large labels Jan 16, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 16, 2025

[Experimental Bot, please feedback here]

Fill In The Commit Message: This PR contains a Commit with an Empty Commit Message. Please fill in the Commit Message with the PR Summary.

This PR does not fully meet the NuttX requirements. While it provides a summary of what is being changed, it lacks crucial details.

Here's what's missing:

  • Summary:

    • Why is this change necessary? What problem does it solve? Is the current errno handling inflexible? How?
    • How exactly does the generation work? Is there a script? Where does the CSV file live? How is it integrated into the build system?
    • Issue references are missing. Even if there isn't a directly related issue, it's good practice to create one to discuss the proposed change before submitting a PR.
  • Impact: This section is completely empty. All points need to be addressed, even if the answer is "NO". Justify the "NO" answers. For example: "Impact on user: NO. This is an internal change to errno generation and does not affect the API." If the answer is "YES," explain the impact in detail. For instance, will existing user code break? Will new configuration options be added?

  • Testing:

    • No build host or target information is provided. What platforms was this tested on? Be specific (e.g., "Linux, x86_64, GCC 12.1", "sim:qemu-x86_64").
    • No test logs are provided. Demonstrate that the change works as intended. Show relevant output before and after the change. Even a simple "printf" demonstrating the generated errno values would be helpful.

The PR needs to provide significantly more detail to be considered complete. Addressing the above points will make it easier for reviewers to understand and evaluate the change.

@yamt
Copy link
Contributor Author

yamt commented Jan 16, 2025

i have a question: is python acceptable for this kind of tools these days?
honestly speaking, i'm not inclined to write this in C.

@raiden00pl
Copy link
Member

raiden00pl commented Jan 16, 2025

i have a question: is python acceptable for this kind of tools these days?
honestly speaking, i'm not inclined to write this in C.

I don't think so, or I missed something. If the build system is to depend on python, then this is a more serious issue to discuss in the community. Or was it introduced quietly and no one noticed?

EDIT: if python is used to generate the file, but not when build process, then it's probably OK?

@yamt
Copy link
Contributor Author

yamt commented Jan 16, 2025

i have a question: is python acceptable for this kind of tools these days?
honestly speaking, i'm not inclined to write this in C.

I don't think so, or I missed something. If the build system is to depend on python, then this is a more serious issue to discuss in the community. Or was it introduced quietly and no one noticed?

EDIT: if python is used to generate the file, but not when build process, then it's probably OK?

the script this PR added (mkerrno.py) is not meant to be used during a build.
i expect a developer who modified errno.csv to run it to regenerate some source files.

otoh, some of existing python scripts in the tools dir seem to be used during a build.
eg. mkallsyms.py, kasan_global.py

@raiden00pl
Copy link
Member

the script this PR added (mkerrno.py) is not meant to be used during a build.
i expect a developer who modified errno.csv to run it to regenerate some source files.

then it's definitely ok.

otoh, some of existing python scripts in the tools dir seem to be used during a build.
eg. mkallsyms.py, kasan_global.py

yeah, I just saw this. So it was introduced without much discussion and even requirements in the documentation hasn't been updated. I see it's only for the case when KASAN is enabled, but this doesn't change the fact that python is now required to build....

@xiaoxiang781216
Copy link
Contributor

@yamt what's the benefit to generate the errno from a csv, but not modify errno.h direclty?

@yamt
Copy link
Contributor Author

yamt commented Jan 18, 2025

@yamt what's the benefit to generate the errno from a csv, but not modify errno.h direclty?

i want to generate other things as well, which are difficult or impossible with C preprocessor.
eg.

#if defined(EPERM)
case EPERM: return NUTTX_EPERM;
#endif

@xiaoxiang781216
Copy link
Contributor

@yamt what's the benefit to generate the errno from a csv, but not modify errno.h direclty?

i want to generate other things as well, which are difficult or impossible with C preprocessor. eg.

#if defined(EPERM)
case EPERM: return NUTTX_EPERM;
#endif

I still can't fully understand what you want to achieve with this code snippet, could you explain more? In most case, it's be better to complish the generation work as macro as we can.

@xiaoxiang781216
Copy link
Contributor

the script this PR added (mkerrno.py) is not meant to be used during a build.
i expect a developer who modified errno.csv to run it to regenerate some source files.

then it's definitely ok.

but why not generate automatically?

otoh, some of existing python scripts in the tools dir seem to be used during a build.
eg. mkallsyms.py, kasan_global.py

yeah, I just saw this. So it was introduced without much discussion and even requirements in the documentation hasn't been updated. I see it's only for the case when KASAN is enabled, but this doesn't change the fact that python is now required to build....

both(kasan.py and mkallsyms.py) are advanced features, which are disabled by default. so python isn't required before you enable these from defconfig.

But, I need mention that shell script(include the tools invoked in it) is hard to work in Windows native environment. If we want to improve the Windows native develop experience, python is the best script we can choice.

@yamt
Copy link
Contributor Author

yamt commented Jan 19, 2025

@yamt what's the benefit to generate the errno from a csv, but not modify errno.h direclty?

i want to generate other things as well, which are difficult or impossible with C preprocessor. eg.

#if defined(EPERM)
case EPERM: return NUTTX_EPERM;
#endif

I still can't fully understand what you want to achieve with this code snippet, could you explain more?

convert host errno to nuttx errno.

In most case, it's be better to complish the generation work as macro as we can.

i can't think of any way to do the equivalent with C preprocessor alone.

@xiaoxiang781216
Copy link
Contributor

@yamt what's the benefit to generate the errno from a csv, but not modify errno.h direclty?

i want to generate other things as well, which are difficult or impossible with C preprocessor. eg.

#if defined(EPERM)
case EPERM: return NUTTX_EPERM;
#endif

I still can't fully understand what you want to achieve with this code snippet, could you explain more?

convert host errno to nuttx errno.

why not simply define EINVAL_LINUX, EINVAL_MACOS, EINVAL_WINDOWS directly, instead through errno_xxx.csv and mkerrno.py.

In most case, it's be better to complish the generation work as macro as we can.

i can't think of any way to do the equivalent with C preprocessor alone.

sorry, I don't see any hard block issue to implement the conversion in c entirely.

@yamt
Copy link
Contributor Author

yamt commented Jan 20, 2025

@yamt what's the benefit to generate the errno from a csv, but not modify errno.h direclty?

i want to generate other things as well, which are difficult or impossible with C preprocessor. eg.

#if defined(EPERM)
case EPERM: return NUTTX_EPERM;
#endif

I still can't fully understand what you want to achieve with this code snippet, could you explain more?

convert host errno to nuttx errno.

why not simply define EINVAL_LINUX, EINVAL_MACOS, EINVAL_WINDOWS directly, instead through errno_xxx.csv and mkerrno.py.

EPREM in the above snippet is the host one. (just use the host errno.h)
i don't see any benefit to have separate definitions for each platforms.

In most case, it's be better to complish the generation work as macro as we can.

i can't think of any way to do the equivalent with C preprocessor alone.

sorry, I don't see any hard block issue to implement the conversion in c entirely.

well, of course, i can keep the csv and script private and only submit the generated C files to nuttx.
i don't see any benefit to hide how these files were actually generated though.

@yamt
Copy link
Contributor Author

yamt commented Jan 20, 2025

the script this PR added (mkerrno.py) is not meant to be used during a build.
i expect a developer who modified errno.csv to run it to regenerate some source files.

then it's definitely ok.

but why not generate automatically?

because

  • i expect that it's very rare to change the errno definitions
  • i expect people are not happy with python requirement for just a build
  • i don't want to write this in C
  • it's cumbersome to update multiple build frameworks (make and cmake)

@xiaoxiang781216
Copy link
Contributor

@yamt what's the benefit to generate the errno from a csv, but not modify errno.h direclty?

i want to generate other things as well, which are difficult or impossible with C preprocessor. eg.

#if defined(EPERM)
case EPERM: return NUTTX_EPERM;
#endif

I still can't fully understand what you want to achieve with this code snippet, could you explain more?

convert host errno to nuttx errno.

why not simply define EINVAL_LINUX, EINVAL_MACOS, EINVAL_WINDOWS directly, instead through errno_xxx.csv and mkerrno.py.

EPREM in the above snippet is the host one. (just use the host errno.h) i don't see any benefit to have separate definitions for each platforms.

In most case, it's be better to complish the generation work as macro as we can.

i can't think of any way to do the equivalent with C preprocessor alone.

sorry, I don't see any hard block issue to implement the conversion in c entirely.

well, of course, i can keep the csv and script private and only submit the generated C files to nuttx. i don't see any benefit to hide how these files were actually generated though.

Does your solution share one errno.csv for all OS or have the seperate errno.csv for each OS? The complex of include/errno.csv and include/errno.h is almost same, so whether we can share the same errno.csv is a key point.

@yamt
Copy link
Contributor Author

yamt commented Jan 20, 2025

@yamt what's the benefit to generate the errno from a csv, but not modify errno.h direclty?

i want to generate other things as well, which are difficult or impossible with C preprocessor. eg.

#if defined(EPERM)
case EPERM: return NUTTX_EPERM;
#endif

I still can't fully understand what you want to achieve with this code snippet, could you explain more?

convert host errno to nuttx errno.

why not simply define EINVAL_LINUX, EINVAL_MACOS, EINVAL_WINDOWS directly, instead through errno_xxx.csv and mkerrno.py.

EPREM in the above snippet is the host one. (just use the host errno.h) i don't see any benefit to have separate definitions for each platforms.

In most case, it's be better to complish the generation work as macro as we can.

i can't think of any way to do the equivalent with C preprocessor alone.

sorry, I don't see any hard block issue to implement the conversion in c entirely.

well, of course, i can keep the csv and script private and only submit the generated C files to nuttx. i don't see any benefit to hide how these files were actually generated though.

Does your solution share one errno.csv for all OS or have the seperate errno.csv for each OS? The complex of include/errno.csv and include/errno.h is almost same, so whether we can share the same errno.csv is a key point.

errno.csv only defines things for nuttx.

i have no plan to have csv files for other OSes. (linux, macos, etc)
just because i don't see any needs for my purposes.

@xiaoxiang781216
Copy link
Contributor

Does your solution share one errno.csv for all OS or have the seperate errno.csv for each OS? The complex of include/errno.csv and include/errno.h is almost same, so whether we can share the same errno.csv is a key point.

errno.csv only defines things for nuttx.

So, why do we introduce errno.csv? It's more simple to modify errno.h directly to add/remove/change errno.

i have no plan to have csv files for other OSes. (linux, macos, etc) just because i don't see any needs for my purposes.

Sorry, I still can't understand how csv could help to fix hostfs issue. With the current patchset, I don't see any benefit to generate errno.h from mkerrno.py and errno.csv.

@yamt
Copy link
Contributor Author

yamt commented Jan 20, 2025

Does your solution share one errno.csv for all OS or have the seperate errno.csv for each OS? The complex of include/errno.csv and include/errno.h is almost same, so whether we can share the same errno.csv is a key point.

errno.csv only defines things for nuttx.

So, why do we introduce errno.csv? It's more simple to modify errno.h directly to add/remove/change errno.

because, as explained in the commit message, i want to generate more than errno.h (and automatically keep them consistent)

i have no plan to have csv files for other OSes. (linux, macos, etc) just because i don't see any needs for my purposes.

Sorry, I still can't understand how csv could help to fix hostfs issue. With the current patchset, I don't see any benefit to generate errno.h from mkerrno.py and errno.csv.

there is no benefit from this PR alone because it only generates errno.h at this point.

@xiaoxiang781216
Copy link
Contributor

could you provide other patch? so I can understand the whole picture.

@yamt
Copy link
Contributor Author

yamt commented Jan 20, 2025

could you provide other patch? so I can understand the whole picture.

ok.

(it might take a bit long to find time to work on this.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: OS Components OS Components issues Area: Tooling Size: L The size of the change in this PR is large
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants