bonsai
/
coreutils
5
4
Fork 3
Code Issues 39 Pull Requests 6 Projects 1 Releases 9 Activity

Added strerror(3) as a Rust library #76

Closed
emma wants to merge 16 commits from strerror into main
pull from: strerror
merge into: bonsai:main
Conversation 4 Commits 16 Files Changed 1 +12 -12
emma commented 2024-03-02 06:16:53 +00:00
Owner
Copy Link

This makes Rust error messages consistent with C error messages.

This makes Rust error messages consistent with C error messages.
emma added the
enhancement
 label 2024-03-02 06:16:53 +00:00
emma added 5 commits 2024-03-02 06:16:53 +00:00
emma requested review from trinity 2024-03-02 06:16:59 +00:00
emma added 1 commit 2024-03-02 06:52:27 +00:00
trinity requested changes 2024-03-02 10:45:11 +00:00
trinity left a comment
Owner
Copy Link

See notes.

See notes.
Makefile Outdated
@ -15,3 +15,2 @@
CC=cc
RUSTC=rustc
CC?=cc
trinity commented 2024-03-02 10:27:45 +00:00
Owner
Copy Link

I really like these Makefile changes.

I really like these Makefile changes.
emma marked this conversation as resolved
src/strerror.rs Outdated
@ -0,0 +13,4 @@
extern "C" { fn strerror(errnum: c_int) -> *mut c_char; }
/* wrapper function for use in Rust */
pub fn c_error(err: std::io::Error) -> String {
trinity commented 2024-03-02 10:44:09 +00:00
Owner
Copy Link

Don't name things after implementation details because implementations change. errormessage or emsg or something would be better. emessage? I don't know.

Don't name things after implementation details because implementations change. `errormessage` or `emsg` or something would be better. `emessage`? I don't know.
emma marked this conversation as resolved
src/strerror.rs Outdated
@ -0,0 +15,4 @@
/* wrapper function for use in Rust */
pub fn c_error(err: std::io::Error) -> String {
/* Get the raw OS error. If it’s None, what the hell is going on‽ */
let error = err.raw_os_error().unwrap_or_else(|| { panic!() }) as c_int;
trinity commented 2024-03-02 10:38:40 +00:00
Owner
Copy Link

For consistency with C conventions error should be errno.

Instead of panicking, set errno to 0. POSIX requires strerror(3p) to have useful function when its argument is 0.

To save work with inevitable localization stuff it may be wise to use strerror_l(3p) instead with the local locale - if we want to support locales.

For consistency with C conventions `error` should be `errno`. Instead of panicking, set errno to 0. POSIX requires [strerror(3p)](https://www.man7.org/linux/man-pages/man3/strerror.3p.html) to have useful function when its argument is 0. To save work with inevitable localization stuff it may be wise to use strerror_l(3p) instead with the local locale - if we want to support locales.
emma commented 2024-03-02 16:51:45 +00:00
Author
Owner
Copy Link

For consistency with C conventions error should be errno.

This is not C and err is not a number.

Instead of panicking, set errno to 0. POSIX requires strerror(3p) to have useful function when its argument is 0.

This is not C, and error is the internal value of an Err variant of a Result, which is an io::Error. Passing this function an io::Error that has no corresponding RawOsError is not the equivalent of passing it 0, it’s more like passing the function a null terminator.

To save work with inevitable localization stuff it may be wise to use strerror_l(3p) instead with the local locale - if we want to support locales.

From strerror(3p):

DESCRIPTION
       For strerror(): The functionality described on this reference page is
       aligned with the ISO C standard. Any conflict between the requirements
       described here and the ISO C standard is unintentional. This volume of
       POSIX.1‐2017 defers to the ISO C standard.

       The strerror() function shall map the error number in errnum to a
       locale-dependent error message string and shall return a pointer to it.
       Typically, the values for errnum come from errno, but strerror() shall
       map any value of type int to a message.
> For consistency with C conventions error should be errno. This is not C and `err` is not a number. > Instead of panicking, set errno to 0. POSIX requires strerror(3p) to have useful function when its argument is 0. This is not C, and error is the internal value of an `Err` variant of a `Result`, which is an `io::Error`. Passing this function an `io::Error` that has no corresponding `RawOsError` is not the equivalent of passing it `0`, it’s more like passing the function a null terminator. > To save work with inevitable localization stuff it may be wise to use strerror_l(3p) instead with the local locale - if we want to support locales. From strerror(3p): ``` DESCRIPTION For strerror(): The functionality described on this reference page is aligned with the ISO C standard. Any conflict between the requirements described here and the ISO C standard is unintentional. This volume of POSIX.1‐2017 defers to the ISO C standard. The strerror() function shall map the error number in errnum to a locale-dependent error message string and shall return a pointer to it. Typically, the values for errnum come from errno, but strerror() shall map any value of type int to a message. ```
emma commented 2024-03-02 21:24:18 +00:00
Author
Owner
Copy Link

For consistency with C conventions error should be errno.

This is not C and err is not a number.

I misunderstood. I will change the error variable to errno if it’ll be more understandable to you.

> > For consistency with C conventions error should be errno. > > This is not C and err is not a number. I misunderstood. I will change the `error` variable to `errno` if it’ll be more understandable to you.
trinity commented 2024-03-02 21:52:26 +00:00
Owner
Copy Link

I'm glad you showed me the strerrror(3p) locale thing.

In person we discussed errno being fine and the panic! being technically unreachable - though I still believe 0 should be passed to strerror(3) instead of a panic.

I'm glad you showed me the strerrror(3p) locale thing. In person we discussed `errno` being fine and the `panic!` being technically unreachable - though I still believe 0 should be passed to strerror(3) instead of a panic.
emma commented 2024-03-19 03:33:26 +00:00
Author
Owner
Copy Link

I have decided to make 0 work but as soon as we can put manual compiler errors in the library we will do that.

I have decided to make `0` work but as soon as we can put manual compiler errors in the library we will do that.
emma marked this conversation as resolved
trinity reviewed 2024-03-02 10:47:20 +00:00
src/strerror.rs Outdated
@ -0,0 +20,4 @@
/* Get a CStr from the error message so that it’s referenced and then
* convert it to an owned value. If the string is not valid UTF-8, return
* that error instead. */
match unsafe { CStr::from_ptr(strerror(error)) }.to_str() {
trinity commented 2024-03-02 10:47:20 +00:00
Owner
Copy Link

You should note somewhere that this can return an empty string in case of an error.

You should note somewhere that this can return an empty string in case of an error.
emma commented 2024-03-02 16:53:42 +00:00
Author
Owner
Copy Link

It cannot do this. It either returns a String derived from the RawOsError or, in the case that the locale-specified error string is not valid UTF-8, returns a String derived from a Utf8Error.

It cannot do this. It either returns a `String` derived from the `RawOsError` or, in the case that the locale-specified error string is not valid UTF-8, returns a `String` derived from a `Utf8Error`.
emma marked this conversation as resolved
emma added 1 commit 2024-03-02 17:17:26 +00:00
emma added 1 commit 2024-03-04 00:35:49 +00:00
emma added 1 commit 2024-03-09 19:42:00 +00:00
emma added 1 commit 2024-03-10 05:03:16 +00:00
emma added 1 commit 2024-03-19 02:47:22 +00:00
emma requested review from trinity 2024-03-19 03:06:24 +00:00
emma added 2 commits 2024-03-19 03:32:08 +00:00
trinity requested changes 2024-03-22 12:20:48 +00:00
trinity left a comment
Owner
Copy Link

I"ve suggested some changes but I really like the intent.

I"ve suggested some changes but I really like the intent.
Makefile Outdated
@ -11,3 +13,4 @@
.PRAGMA: posix_202x # future POSIX standard support à la pdpmake(1)
.PRAGMA: command_comment # breaks without this?
DESTDIR=./dist
trinity commented 2024-03-10 15:17:18 +00:00
Owner
Copy Link

For readability, spaces between the assignment operators and their names and values would be nice. e.g. DESTDIR = ./dist (though I would forego the ./ as Makefiles are conventionally run relative to $PWD anyway).

For readability, spaces between the assignment operators and their names and values would be nice. e.g. `DESTDIR = ./dist` (though I would forego the `./` as Makefiles are conventionally run relative to `$PWD` anyway).
emma marked this conversation as resolved
Makefile Outdated
@ -32,3 +42,1 @@
mkdir -p dist/bin dist/share/man/man1
cp build/bin/* dist/bin/
cp docs/*.1 dist/share/man/man1/
mkdir -p $(DESTDIR)$(PREFIX)/bin $(DESTDIR)$(PREFIX)/share/man/man1
trinity commented 2024-03-10 15:21:12 +00:00
Owner
Copy Link

Consecutive directory delimiters are ignored when evaluating UNIX paths; /// is an unnecessarily wordy equivalent to / when used as a pathname. $(DESTDIR)/$(PREFIX)/bin and so on is preferable, though, to ensure that hasty modifications won't write to a distbuildbin or something. Mistakes happen!

The names PREFIX and DESTDIR sound equivalent to me. On face value I would expect the prefix to come first.

Consecutive directory delimiters are ignored when evaluating UNIX paths; `///` is an unnecessarily wordy equivalent to `/` when used as a pathname. `$(DESTDIR)/$(PREFIX)/bin` and so on is preferable, though, to ensure that hasty modifications won't write to a `distbuildbin` or something. Mistakes happen! The names `PREFIX` and `DESTDIR` sound equivalent to me. On face value I would expect the prefix to come first.
emma commented 2024-03-24 19:21:54 +00:00
Author
Owner
Copy Link

Historically, $DESTDIR is a directory inside the tree (in this case, dist/) to which programs are installed in a phony root filesystem. Then, when make install is called, the $PREFIX is already baked into $DESTDIR and you can copy it straight into the root filesystem.

[Historically](https://people.freebsd.org/~rafan/ph/porting-prefix.html), `$DESTDIR` is a directory inside the tree (in this case, `dist/`) to which programs are installed in a phony root filesystem. Then, when `make install` is called, the `$PREFIX` is already baked into `$DESTDIR` and you can copy it straight into the root filesystem.
emma marked this conversation as resolved
Makefile
@ -37,3 +47,2 @@
install: dist
mkdir -p $(PREFIX)
cp -r dist/* $(PREFIX)/
cp -r $(DESTDIR)/* /
trinity commented 2024-03-10 15:22:10 +00:00
Owner
Copy Link

Installation to a given PREFIX should be possible.

Installation to a given `PREFIX` should be possible.
emma commented 2024-03-24 19:05:51 +00:00
Author
Owner
Copy Link

That is what this does

That is what this does
emma commented 2024-03-29 22:31:43 +00:00
Author
Owner
Copy Link

@trinity if you have no more questions, may you approve this for merging?

@trinity if you have no more questions, may you approve this for merging?
emma marked this conversation as resolved
src/test.rs
@ -0,0 +1,10 @@
extern crate strerror;
trinity commented 2024-03-22 12:20:00 +00:00
Owner
Copy Link

What's the purpose of this file?

What's the purpose of this file?
emma commented 2024-03-24 19:05:42 +00:00
Author
Owner
Copy Link

Mistake.

Mistake.
emma marked this conversation as resolved
trinity added 1 commit 2024-03-24 00:18:16 +00:00
emma added 1 commit 2024-03-24 19:17:15 +00:00
emma commented 2024-03-24 19:18:30 +00:00
Author
Owner
Copy Link

@trinity: in the future, do not make commits without PGP signing them, and do not merge main into branches that have pull requests open to main.

@trinity: in the future, do not make commits without PGP signing them, and do not merge main into branches that have pull requests open to main.
emma added 1 commit 2024-03-24 19:29:26 +00:00
trinity commented 2024-03-25 00:49:15 +00:00
Owner
Copy Link

@trinity: in the future, do not make commits without PGP signing them, and do not merge main into branches that have pull requests open to main.

I think I misclicked, I don't remember doing that and I wouldn't commit to a branch I didn't create. Sorry about that.

> @trinity: in the future, do not make commits without PGP signing them, and do not merge main into branches that have pull requests open to main. I think I misclicked, I don't remember doing that and I wouldn't commit to a branch I didn't create. Sorry about that.
trinity reviewed 2024-03-30 00:53:27 +00:00
docs/mm.1
@ -1,76 +0,0 @@
.\" Copyright (c) 2024 DTB <trinity@trinity.moe>
trinity commented 2024-03-30 00:53:27 +00:00
Owner
Copy Link

Would merging this branch nuke mm(1)?

Would merging this branch nuke mm(1)?
emma commented 2024-03-30 01:11:37 +00:00
Author
Owner
Copy Link

I think so, I can fix it though. Just approve it whenever you think it’s ready.

I think so, I can fix it though. Just approve it whenever you think it’s ready.
emma marked this conversation as resolved
emma commented 2024-03-30 01:27:09 +00:00
Author
Owner
Copy Link

Merged.

Merged.
emma closed this pull request 2024-03-30 01:27:09 +00:00
trinity commented 2024-03-30 18:59:07 +00:00
Owner
Copy Link

I gave the okay verbally as I was too tired to approve it digitally.

I gave the okay verbally as I was too tired to approve it digitally.

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No reviewers
trinity
Labels
Clear labels   
bug

Something is not working

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
help wanted

Need some help

  
invalid

Something is wrong

  
joke

you are silly!

  
question

More information is needed

  
wontfix

This won't be fixed

No Label
bug
duplicate
enhancement
help wanted
invalid
joke
question
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: bonsai/coreutils#76
No description provided.
Delete Branch "strerror"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?