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

Remove support for env and process functionality #17

Merged
merged 2 commits into from
Feb 20, 2022

Conversation

Meziu
Copy link
Owner

@Meziu Meziu commented Feb 19, 2022

Related to #6

Removed process support and changed env functions to avoid unsupported functionality. Tell me if anything else is left to remove, we can do one big cleanup here.

@AzureMarker @ian-h-chamberlain

Copy link

@AzureMarker AzureMarker left a comment

Choose a reason for hiding this comment

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

I did test the freestanding functions in process (id, exit, and abort). id returns a really big, static-looking ID that didn't change across runs. Both exit and abort caused the app to quit to the home screen with a dialog saying it crashed and needs to restart.

I also double checked that we can't spawn processes, and it fails to link due to missing symbols (which is expected).

So, I think it's good that we're disabling the process module. However, see the comments about the other functions, since I don't think we should disable them.

@@ -560,7 +560,7 @@ pub fn unsetenv(n: &OsStr) -> io::Result<()> {
}
}

#[cfg(not(target_os = "espidf"))]
#[cfg(not(any(target_os = "espidf", target_os = "horizon")))]
pub fn page_size() -> usize {
unsafe { libc::sysconf(libc::_SC_PAGESIZE) as usize }

Choose a reason for hiding this comment

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

This is actually supported, and is used by std threads. See rust3ds/shim-3ds#11

@@ -129,12 +129,12 @@ pub fn error_string(errno: i32) -> String {
}
}

#[cfg(target_os = "espidf")]
#[cfg(any(target_os = "espidf", target_os = "horizon"))]
pub fn getcwd() -> io::Result<PathBuf> {
Ok(PathBuf::from("/"))

Choose a reason for hiding this comment

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

I was able to run an example that successfully reads the current dir and changes the directory:

println!("Current dir: {:?}", std::env::current_dir());
println!("Change dir result: {:?}", std::env::set_current_dir("/3ds"));
println!("Current dir: {:?}", std::env::current_dir());

I don't think we should remove this.

@AzureMarker
Copy link

AzureMarker commented Feb 20, 2022

Interestingly, libctru implements exit, but when I tested it crashed the process to the home screen (with an error dialog).
https://github.com/devkitPro/libctru/blob/c83c12357e52c30526c7aefaa263a6c86baeed27/libctru/source/system/syscalls.c#L98

@Meziu
Copy link
Owner Author

Meziu commented Feb 20, 2022

Oh wow, env is way more supported than I thought. Even EnvVars are present. We could do some cool stuff with this, like setting HOME to /3ds and such, though that would be something for ctru::init.

Edit: I'm not saying we should do the HOME thing in particular, but it can be useful for some things one would only expect present in env vars.

@Meziu
Copy link
Owner Author

Meziu commented Feb 20, 2022

Yes, env fully works and process has been removed. I can tick one off the #6 problems. Again, we should check for other functions and things to remove, but this is good for now.

@Meziu Meziu merged this pull request into horizon-std Feb 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants