port stest to rust #47

benjaminedwardwebb · 2023-01-13T14:17:39Z

I took the opportunity to learn some rust (something I'd been meaning to do) and port stest over from C, as suggested in this issue. Hopefully the rust looks reasonable, but open to any advice and tips of course.

Since stest was the only remaining C artifact, this pull request also drops all references to the C toolchain from the makefile and build. Yay, less C and simpler build!

verification

I wrote some integration tests for stest. They're not perfect but they cover a lot of the basic cases.

I also ran modified versions of dmenu_path and dmenu_run that pointed to the rust-based stest built locally on my x86_64 GNU/Linux host, and dmenu ran as expected.

benjaminedwardwebb · 2023-01-19T02:57:08Z

src/stest/tests/set-up-block-special-file

+# main : () -> string
+# Return a path to a block special file that exists on most systems.
+function main {
+	echo "/dev/disk/by-diskseq/1" || return 1


I did my best to come up with a file I thought would be an most unix machines, but I'm not the most familiar with linux and don't know how well this choice will actually work out.

Maybe there's an easy way to construct a block special file that we can use instead?

I believe that /dev/disk/by-diskseq/1 works here. Good job finding this.

benjaminedwardwebb · 2023-01-19T02:58:00Z

src/stest/tests/set-up-character-special-file

+# main : () -> string
+# Return a path to a character special file that exists on most systems.
+function main {
+	echo "/dev/tty" || return 1


Same as above. It bothers me that we're making assumptions about existing files on the file system in this test and not just creating the test conditions ourselves.

Neither /dev/tty nor /dev/disk/by-diskseq/1 are actually files; they are guaranteed to exist by Linux.

This statement is almost certainly wrong in some crazy edge case, but the assumption is reasonable. It's appropriate to go forward this way and change if someone files a bug down the road.

This statement is almost certainly wrong in some crazy edge case, but the assumption is reasonable. It's appropriate to go forward this way and change if someone files a bug down the road.

I agree. I'll plan to move forward with this until something better comes up.

One crazy edge-case that's on my mind, however, is the sandboxed environment of a nix build. But that can come later.

Do nix builds commonly run tests as well? If not, this is a non-issue.

I expect that many nix builds inside of the nixpkgs repository do end up skipping tests for one reason or another, however it requires an explicit disablement and I think it's probably discouraged.

For example, I disabled the test phase of dmenu-rs's make build when checking it into nixpkgs here. There was some discussion in the PR from contributors about ways to emulate X to enable the tests to run, but ultimately it proved too much to hack around. But I hope that can change in the future.

Shizcow

Sorry for just now getting to this. There are a few things that should be changed, but the overall content is great.

Shizcow · 2023-02-04T23:15:14Z

src/stest/src/config.rs

+    /// Test that files have their set-group-ID flag set.
+    #[arg(short = 'g')]
+    pub requires_each_file_has_set_group_id: bool,
+    // Using -h here works and matches the original stest, however it overrides one of clap's


This is alright. Feature parity is paramount.

Shizcow · 2023-02-04T23:58:09Z

makefile

-ifeq ($(CC),)
-	CC = cc
-endif
-


Removing CC entirely is good, but it causes checkdeps.sh to fail. This script looks for a C compiler and ensures that xlib/fontconfig/etc libraries are available. These libraries (and their corresponding C headers) are later used by bindgen so that we can call native X11 functions. That said, bindgen properly(?) selects the default/overridden C compiler. So, the only reason an explicit C compiler is currently required is for checkdeps.sh.

I think the correct way to fix this is to:

Remove checkdeps.sh and all references to it

In headers/build.rs, separate fontconfig.h and xinerama.h into separate headers (And while we're at it, figure out how to disable the Xinerama part if Xinerama is disabled in the makefile).

Add error info to headers/build.rs. Eg replace Unable to generate bindings_main with Unable to generate fontconfig bindings. Do you have fontconfig installed?

Make sure all three (or two, if Xinerame is disabled) header files are included in the proper places.

I didn't realize what failure you were talking about until returning to this just today, because CC happened to be set in my environment without my realizing and everything I ran seemed to work.

I've restored the bits of the make build that set CC appropriately so that the config subproject and checkdeps.sh script will work.

I agree it should be removed and would have liked to follow the nice plan you laid out, I just don't have the time or energy at the moment and wanted to get this PR into a merge-able state.

Will keep it in mind though if I find some time to work on this project again! That is, if you don't get to it first. :)

Shizcow · 2023-02-05T00:02:50Z

src/Cargo.toml

Nitpick: Can this have the same indentation?

Of course. Sorry about this -- tabs not spaces. :)

On another note, probably we could enforce this stuff with rustfmt or some other pre-commit hook?

Shizcow · 2023-02-05T00:03:16Z

src/stest/Cargo.toml

+version = "0.0.0"
+edition = "2021"
+
+# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html


Nitpick: Can this comment be removed?

Shizcow · 2023-02-05T00:03:32Z

src/stest/Cargo.toml

@@ -0,0 +1,9 @@
+[package]
+name = "stest"
+version = "0.0.0"


When I run the new stest binary as stest --version it reports 0.0.0. Can you have clap reference env!("VERSION") to get the version info when passed --version?

You might need to override the --version command for this.

Extremely pleased to discover that this just works:

#[command(author, version = env!("VERSION"), about, long_about)]

Clap really is awesome :)

Shizcow · 2023-02-05T00:28:42Z

src/stest/src/file.rs

+
+/// Wraps a [PathBuf] for use in stest.
+///
+/// These files are assumed to be on a unix system. Error-handling is simple: always panic.


Just unwrapping makes for very messy errors. In any place you actually expect to see an error, you should at least expect("A better error message"). Realistically, expect isn't as good as just printing an error and exiting, as it also prints source line info (not clean).

In the case of this program, I believe the right way of doing it would be to return a Result<bool, String> from all of the test functions. true or false are the test results. Error would then be a problem performing the test. For example, Problem checking hidden status: File has an empty path.

You could also return Result<bool,std::error::Error> so that you can mix custom string errors with io errors. std::error::Error implements display so you can just print and exit at top level.

Finally got a version that folds in the Result type throughout. :)

Shizcow · 2023-02-05T00:35:14Z

src/stest/src/lib.rs

+        let passes_all_tests =
+            (!self.config.requires_each_file_is_hidden || file.is_hidden()) &&
+            (!self.config.requires_each_file_is_block_special || file.is_block_special()) &&
+            (!self.config.requires_each_file_is_character_special || file.is_character_special()) &&
+            (!self.config.requires_each_file_is_directory || file.is_directory()) &&
+            (!self.config.requires_each_file_exists || file.exists()) &&
+            (!self.config.requires_each_file_is_file || file.is_file()) &&
+            (!self.config.requires_each_file_has_set_group_id || file.has_set_group_id()) &&
+            (!self.config.requires_each_file_is_symbolic_link || file.is_symbolic_link()) &&
+            (self.optionally_test_if_newer_than_oldest_file(file)) &&
+            (self.optionally_test_if_older_than_newest_file(file)) &&
+            (!self.config.requires_each_file_is_pipe || file.is_pipe()) &&
+            (!self.config.requires_each_file_is_readable || file.is_readable()) &&
+            (!self.config.requires_each_file_has_size_greater_than_zero || file.has_size_greater_than_zero()) &&
+            (!self.config.requires_each_file_has_set_user_id || file.has_set_user_id()) &&
+            (!self.config.requires_each_file_is_writable || file.is_writable()) &&
+            (!self.config.requires_each_file_is_executable || file.is_executable());


If you make all of these return Result, you can just ? on all of them and have this function also return a Result<bool,String>. That will short circuit properly.

I like this idea, and I've spent a little bit of time (not as much as I was hoping to, though) trying to push out the "evaluation" of the Result type into the outer shell of the app -- in either main or lib::App.run.

However, it's taking me some time to work around certain issues, such as the fact that filter can only accept a bool returning function, not a Result<bool, _>. I think this means I have to wrap the iteration itself in a try or Result-like expression to get the Result type back. But seems worth it to get a single layer of error-handling at the top-level.

Shizcow · 2023-02-05T00:39:49Z

src/stest/src/file.rs

+    /// See: https://stackoverflow.com/a/50045872/8732788
+    /// See: https://en.wikipedia.org/wiki/Setuid
+    pub fn has_set_group_id(&self) -> bool {
+        fn has_set_group_id(mode: u32) -> bool { mode & 0o2000 != 0 }


Shizcow · 2023-02-05T00:46:52Z

src/stest/src/file.rs

+}
+
+impl FromStr for File {
+    type Err = std::convert::Infallible;


Why is this std::convert::Infallible?

This seemed strange to me too, but apparently that is the Err type for FromStr on PathBuf. I guess PathBuf.from_str(str) always succeeds?

Shizcow · 2023-02-05T00:49:10Z

src/stest/src/main.rs

+    match result {
+        Ok(_) => ExitCode::SUCCESS,
+        Err(_) => ExitCode::FAILURE,
+    }


If you made all of the tests return Result, here is where you would print out the error before exiting. You'd just Ok(true) to success, Ok(false) to failure, and Err(e) to print out the error and return failure.

benjaminedwardwebb · 2023-02-11T01:22:57Z

Sorry for just now getting to this.

Not a problem at all. You're certainly under no obligation to maintain open-source projects if you're otherwise busy.

I appreciate the suggestions and comments you've made. I've responded to some of the simpler comments, and am working on addressing the issues you've called out regarding error-handling and the checkdeps.sh script. I should have time tomorrow to sort through some of it.

Have a good weekend. :)

Shizcow · 2023-02-13T17:16:53Z

I appreciate the suggestions and comments you've made. I've responded to some of the simpler comments, and am working on addressing the issues you've called out regarding error-handling and the checkdeps.sh script. I should have time tomorrow to sort through some of it.

Not a problem. If you can't/don't want to fix the checkdeps/other issues I've mentioned I can do that in a week or two myself. Looking forward to getting this merged 😄

benjaminedwardwebb · 2023-02-17T15:25:01Z

I appreciate the suggestions and comments you've made. I've responded to some of the simpler comments, and am working on addressing the issues you've called out regarding error-handling and the checkdeps.sh script. I should have time tomorrow to sort through some of it.

Not a problem. If you can't/don't want to fix the checkdeps/other issues I've mentioned I can do that in a week or two myself. Looking forward to getting this merged smile

I ended up not having the time I thought I would last weekend to finish this. I won't be able to pick it up until next week some time, maybe next weekend.

Please feel free to merge anything else in the mean time, I'm happy to rebase over any changes. Alternatively, if you want to merge sooner you're welcome to merge it as-is and I can continue to work on the other pieces in a follow-on.

Adds `cargo test` to the make test target.

Since stest was the only remaining C artifact, this commit also drops all references to the C toolchain from the makefile and build. Yay, less C and simpler build!

benjaminedwardwebb · 2023-03-13T01:22:03Z

@Shizcow I think I've addressed your main concerns now, including the checkdeps.sh/$CC issue.

Anything holding this PR from being merged?

Shizcow · 2023-03-13T18:18:24Z

@benjaminedwardwebb Everything looks great. There are a few followup items that need addressed (checkdeps.sh is still broken and will cause build to fail on some systems), but I'll be able to fix these today after merging.

Thank you SO much for your work on this. This is a great contribution that makes a measurable impact on this project. Let me know if you need anything from me in the future (including other NIX-related work).

benjaminedwardwebb force-pushed the port-stest-to-rust branch from 82d6e04 to 5f80a7d Compare January 13, 2023 14:21

Shizcow mentioned this pull request Jan 18, 2023

don't ignore Cargo.lock #46

Merged

benjaminedwardwebb force-pushed the port-stest-to-rust branch 2 times, most recently from e35c92e to 9be31d0 Compare January 19, 2023 02:43

benjaminedwardwebb commented Jan 19, 2023

View reviewed changes

benjaminedwardwebb marked this pull request as ready for review January 19, 2023 02:58

Shizcow requested changes Feb 5, 2023

View reviewed changes

Shizcow mentioned this pull request Feb 5, 2023

support aarch64-linux #45

Open

benjaminedwardwebb force-pushed the port-stest-to-rust branch 2 times, most recently from b59da6c to dbfd026 Compare February 23, 2023 01:41

add cargo test to make test

fa4df79

Adds `cargo test` to the make test target.

benjaminedwardwebb force-pushed the port-stest-to-rust branch 2 times, most recently from 44e4819 to 685d0ef Compare February 23, 2023 01:47

ports stest to rust

7266ba5

Since stest was the only remaining C artifact, this commit also drops all references to the C toolchain from the makefile and build. Yay, less C and simpler build!

benjaminedwardwebb force-pushed the port-stest-to-rust branch from 685d0ef to 7266ba5 Compare March 13, 2023 01:17

Shizcow merged commit e8497ca into Shizcow:master Mar 13, 2023

Shizcow mentioned this pull request Mar 13, 2023

Dependabot Issues #49

Closed

benjaminedwardwebb deleted the port-stest-to-rust branch September 16, 2023 03:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

port stest to rust #47

port stest to rust #47

benjaminedwardwebb commented Jan 13, 2023 •

edited

Loading

benjaminedwardwebb Jan 19, 2023 •

edited

Loading

Shizcow Feb 5, 2023

benjaminedwardwebb Jan 19, 2023

Shizcow Feb 5, 2023

benjaminedwardwebb Feb 11, 2023

Shizcow Feb 13, 2023

benjaminedwardwebb Feb 17, 2023

Shizcow left a comment

Shizcow Feb 4, 2023

Shizcow Feb 4, 2023

benjaminedwardwebb Mar 13, 2023

Shizcow Feb 5, 2023

benjaminedwardwebb Feb 23, 2023

Shizcow Feb 5, 2023

Shizcow Feb 5, 2023

Shizcow Feb 5, 2023

benjaminedwardwebb Feb 11, 2023

Shizcow Feb 5, 2023

Shizcow Feb 5, 2023

Shizcow Feb 5, 2023

benjaminedwardwebb Feb 24, 2023 •

edited

Loading

Shizcow Feb 5, 2023

benjaminedwardwebb Feb 17, 2023 •

edited

Loading

Shizcow Feb 5, 2023

Shizcow Feb 5, 2023

benjaminedwardwebb Feb 11, 2023 •

edited

Loading

Shizcow Feb 5, 2023

benjaminedwardwebb commented Feb 11, 2023

Shizcow commented Feb 13, 2023

benjaminedwardwebb commented Feb 17, 2023 •

edited

Loading

benjaminedwardwebb commented Mar 13, 2023

Shizcow commented Mar 13, 2023

port stest to rust #47

port stest to rust #47

Conversation

benjaminedwardwebb commented Jan 13, 2023 • edited Loading

verification

benjaminedwardwebb Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shizcow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminedwardwebb Feb 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminedwardwebb Feb 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminedwardwebb Feb 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminedwardwebb commented Feb 11, 2023

Shizcow commented Feb 13, 2023

benjaminedwardwebb commented Feb 17, 2023 • edited Loading

benjaminedwardwebb commented Mar 13, 2023

Shizcow commented Mar 13, 2023

benjaminedwardwebb commented Jan 13, 2023 •

edited

Loading

benjaminedwardwebb Jan 19, 2023 •

edited

Loading

benjaminedwardwebb Feb 24, 2023 •

edited

Loading

benjaminedwardwebb Feb 17, 2023 •

edited

Loading

benjaminedwardwebb Feb 11, 2023 •

edited

Loading

benjaminedwardwebb commented Feb 17, 2023 •

edited

Loading