feat: Replace use of the `libc` crate with the `rustix` crate #582

thaodt · 2023-06-26T11:04:22Z

This PR is served for this issue #521.

As discussed some days ago, I will leave the time functions as it is for the time being, as these can easily be ported to Inko itself using the new FFI.

Also, there's a function I can't find an alternative to in rustix, see the code below:

inko/rt/src/runtime.rs

Lines 31 to 43 in 59578a1

    
           fn ignore_sigpipe() { 
        
               // Broken pipe errors default to terminating the entire program, making it 
        
               // impossible to handle such errors. This is especially problematic for 
        
               // sockets, as writing to a socket closed on the other end would terminate 
        
               // the program. 
        
               // 
        
               // While Rust handles this for us when compiling an executable, it doesn't 
        
               // do so when compiling it to a static library and linking it to our 
        
               // generated code, so we must handle this ourselves. 
        
               unsafe { 
        
                   libc::signal(libc::SIGPIPE, libc::SIG_IGN); 
        
               } 
        
           }

Therefore, I still keep it in libc. If you have any ideas, please review and advise, I will update my PR.

How Has This Been Tested?

Existing tests pass.

What process can a PR reviewer use to test or verify this change?

Check the new rustix replacements are correct.

thaodt · 2023-07-04T02:30:06Z

It seems it didn't assign to you 😅 once I changed it into "Open for review". @yorickpeterse could you please help me take a look on my PR?

yorickpeterse

Regarding the signal() call:

We can just use an extern "C" block. We only need this on Unix platforms, and the SIG* constants have the same values on the different Unix systems. This way we don't need to keep the direct dependency on libc around.

Cargo.toml

rt/Cargo.toml

rt/src/memory_map.rs

rt/src/runtime.rs

rt/src/runtime/time.rs

rt/src/socket/socket_address.rs

thaodt · 2023-07-04T12:18:28Z

Thank for your review @yorickpeterse 😄 i will update soon

thaodt · 2023-07-06T03:09:28Z

@yorickpeterse ping you again in case you maybe missed it 😛

rt/src/runtime.rs

thaodt · 2023-07-06T13:49:33Z

@yorickpeterse I've pushed the fix. Could you please take a look again? Hopefully my PR can gooo bruuhh

rt/src/runtime.rs

rt/src/scheduler/mod.rs

rt/src/socket/socket_address.rs

rt/src/runtime.rs

yorickpeterse · 2023-07-07T23:28:13Z

@thaodt I left some more comments. Once the necessary changes are made, please rebase the commits into a single one and stick a Changelog trailer at the end of the commit, this way it will make its way into the changelog. You can find some details on that here and here 😃

thaodt · 2023-07-10T07:21:59Z

@thaodt I left some more comments. Once the necessary changes are made, please rebase the commits into a single one and stick a Changelog trailer at the end of the commit, this way it will make its way into the changelog. You can find some details on that here and here smiley

I've pushed the fix, change the commit message following the guideline. Please help me review @yorickpeterse

yorickpeterse

I've made three small formatting changes (just some wrapping of comments, which rustfmt doesn't do, and some whitespace), the rest looks fine. I think clippy will have some complaints, but I'll add the necessary changes to the commit myself. Thanks! 😃

In the interest of reducing 3rd-party dependencies and maintaining control over our code, I would not only like to restore our old polling code in favour of the polling crate (inko-lang#344), but also switch to using rustix. The benefit of using rustix is that at least on Linux we can avoid libc calls. While this may result in a performance improvement, it's really more about slowly reducing our dependencies on 3rd-party libraries, including libc. The API might also be a bit more pleasant to use compared to using the libc crate directly. Changelog: changed

yorickpeterse · 2023-07-10T23:51:08Z

@thaodt Thanks for working on this! 🎉

thaodt marked this pull request as draft June 26, 2023 11:04

thaodt force-pushed the feat/rustix branch from c1252b6 to 55c04f6 Compare June 28, 2023 07:35

thaodt changed the title ~~[WIP] feat: Replace use of the libc crate with the rustix crate~~ feat: Replace use of the libc crate with the rustix crate Jul 3, 2023

thaodt marked this pull request as ready for review July 3, 2023 13:53

yorickpeterse self-assigned this Jul 4, 2023

yorickpeterse self-requested a review July 4, 2023 10:57

yorickpeterse requested changes Jul 4, 2023

View reviewed changes

yorickpeterse assigned thaodt and unassigned yorickpeterse Jul 4, 2023

thaodt requested a review from yorickpeterse July 5, 2023 14:12

yorickpeterse requested changes Jul 6, 2023

View reviewed changes

rt/src/runtime.rs Outdated Show resolved Hide resolved

thaodt force-pushed the feat/rustix branch from 53dcb3b to f822284 Compare July 6, 2023 13:47

thaodt requested a review from yorickpeterse July 6, 2023 13:48

yorickpeterse requested changes Jul 7, 2023

View reviewed changes

yorickpeterse added feature New things to add to Inko, such as a new standard library module runtime Changes related to the Rust-based runtime library labels Jul 7, 2023

thaodt force-pushed the feat/rustix branch from f822284 to dc8474f Compare July 10, 2023 07:20

thaodt requested a review from yorickpeterse July 10, 2023 07:22

yorickpeterse approved these changes Jul 10, 2023

View reviewed changes

yorickpeterse force-pushed the feat/rustix branch 2 times, most recently from c9d4a51 to 11ea13f Compare July 10, 2023 23:23

yorickpeterse force-pushed the feat/rustix branch from 11ea13f to 7f03150 Compare July 10, 2023 23:29

yorickpeterse merged commit f2379c6 into inko-lang:main Jul 10, 2023

yorickpeterse mentioned this pull request Jul 10, 2023

Replace use of the libc crate with the rustix crate #521

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Replace use of the `libc` crate with the `rustix` crate #582

feat: Replace use of the `libc` crate with the `rustix` crate #582

thaodt commented Jun 26, 2023 •

edited

Loading

thaodt commented Jul 4, 2023 •

edited

Loading

yorickpeterse left a comment

thaodt commented Jul 4, 2023

thaodt commented Jul 6, 2023

thaodt commented Jul 6, 2023 •

edited

Loading

yorickpeterse commented Jul 7, 2023 •

edited

Loading

thaodt commented Jul 10, 2023

yorickpeterse left a comment

yorickpeterse commented Jul 10, 2023

	fn ignore_sigpipe() {
	// Broken pipe errors default to terminating the entire program, making it
	// impossible to handle such errors. This is especially problematic for
	// sockets, as writing to a socket closed on the other end would terminate
	// the program.
	//
	// While Rust handles this for us when compiling an executable, it doesn't
	// do so when compiling it to a static library and linking it to our
	// generated code, so we must handle this ourselves.
	unsafe {
	libc::signal(libc::SIGPIPE, libc::SIG_IGN);
	}
	}

feat: Replace use of the libc crate with the rustix crate #582

feat: Replace use of the libc crate with the rustix crate #582

Conversation

thaodt commented Jun 26, 2023 • edited Loading

How Has This Been Tested?

What process can a PR reviewer use to test or verify this change?

thaodt commented Jul 4, 2023 • edited Loading

yorickpeterse left a comment

Choose a reason for hiding this comment

thaodt commented Jul 4, 2023

thaodt commented Jul 6, 2023

thaodt commented Jul 6, 2023 • edited Loading

yorickpeterse commented Jul 7, 2023 • edited Loading

thaodt commented Jul 10, 2023

yorickpeterse left a comment

Choose a reason for hiding this comment

yorickpeterse commented Jul 10, 2023

feat: Replace use of the `libc` crate with the `rustix` crate #582

feat: Replace use of the `libc` crate with the `rustix` crate #582

thaodt commented Jun 26, 2023 •

edited

Loading

thaodt commented Jul 4, 2023 •

edited

Loading

thaodt commented Jul 6, 2023 •

edited

Loading

yorickpeterse commented Jul 7, 2023 •

edited

Loading