Split up utils module #138

mkroening · 2021-07-11T19:03:04Z

Binaries get a much smaller bin_utils and other helper functions are
replaced/moved.

mkroening · 2021-07-12T19:07:24Z

Rebased on latest master.

codecov · 2021-07-12T21:08:44Z

Codecov Report

Merging #138 (6715a30) into master (22632af) will decrease coverage by 0.96%.
The diff coverage is 25.31%.

@@            Coverage Diff             @@
##           master     #138      +/-   ##
==========================================
- Coverage   33.51%   32.55%   -0.97%     
==========================================
  Files          16       16              
  Lines        3932     3831     -101     
==========================================
- Hits         1318     1247      -71     
+ Misses       2614     2584      -30

Impacted Files	Coverage Δ
src/bin/uhyve.rs	`0.45% <0.00%> (+<0.01%)`	⬆️
src/error.rs	`16.66% <ø> (ø)`
src/gdb_parser.rs	`64.77% <0.00%> (-0.34%)`	⬇️
src/linux/gdb.rs	`0.00% <ø> (ø)`
src/utils.rs	`49.09% <44.44%> (-13.90%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22632af...6715a30. Read the comment docs.

mkroening · 2021-07-28T13:20:20Z

This is ready for review now.

jounathaen · 2021-07-28T13:23:34Z

generate_test_coverage.sh

-        echo "ERROR: doc wasn't build exiting"
-        exit -1
-    fi
+    # Checks if doctests were actually built - currently we don't have any though


Ok, I've now read the comment.
I don't know if we should rather:

remove doctests (seriously, we won't have any in the near to middle future)

keep the code

If you want, I can remove this code, but I'd rather keep it. That would make this coverage config more complete.

src/bin_utils.rs

jounathaen · 2021-07-28T13:30:02Z

In general LGTM!
One minor thing: If we only have src/bin_utils.rs now, why not keep the name src/utils.rs?

mkroening · 2021-07-28T13:37:12Z

If we only have src/bin_utils.rs now, why not keep the name src/utils.rs?

That name came up naturally. Before, we just put all kind of utility functions in utils, even though some were used by the frontend only and some were used in different places in the library. I moved the utilities for the binary to bin_utils. The utility functions for the library turned out not to be needed in a central place and could be moved to different places.

So I would like to distinguish between bin_utils and utils but there just don't happen to be any utils left now.

jounathaen · 2021-07-28T16:52:53Z

Hmm, for me util feels more natural. Now I wonder, which bin you are talking about.
But not that this is a blocker.
Let's wait a few days if @stlankes has something to add, otherwise I'll merge this.

mkroening · 2021-07-28T18:37:18Z

Hmm, for me util feels more natural. Now I wonder, which bin you are talking about.

Well we could also make this a module which only lives in the binary and not in the library (src/bin/uhyve/utils.rs). But since we plan with the possibility of having multiple frontends, sharing this via the library might make sense. But making clear, that this is only used by the frontend and does not matter to the backend is important, I think.

jounathaen · 2021-08-20T14:28:15Z

bors r+

138: Split up utils module r=jounathaen a=mkroening Binaries get a much smaller bin_utils and other helper functions are replaced/moved. Co-authored-by: Martin Kröning <mkroening@posteo.net>

Binaries get a much smaller bin_utils and other helper functions are replaced/moved.

bors · 2021-08-20T14:28:45Z

Canceled.

jounathaen · 2021-08-20T14:29:00Z

bors r+

138: Split up utils module r=jounathaen a=mkroening Binaries get a much smaller bin_utils and other helper functions are replaced/moved. Co-authored-by: Martin Kröning <mkroening@posteo.net>

bors · 2021-08-20T16:30:32Z

Timed out.

jounathaen · 2021-08-20T17:26:01Z

#@$%! 🤬🤬🤬

bors r+

138: Split up utils module r=jounathaen a=mkroening Binaries get a much smaller bin_utils and other helper functions are replaced/moved. Co-authored-by: Martin Kröning <mkroening@posteo.net>

bors · 2021-08-20T17:29:59Z

Build failed:

Cargo tests (macOS-latest, nightly)

mkroening · 2021-08-21T12:11:53Z

bors retry

bors · 2021-08-21T12:22:09Z

Build succeeded:

140: Rework error types r=jounathaen a=mkroening Depends on #138. This heavily reworks the error handling, introducing finer separate error types instead of one uber-error. Co-authored-by: Martin Kröning <mkroening@posteo.net>

mkroening self-assigned this Jul 11, 2021

This was referenced Jul 12, 2021

Rework error types #140

Merged

[TEST] #138 #141

Closed

mkroening force-pushed the utils branch from d3726fe to bb99920 Compare July 12, 2021 19:07

mkroening force-pushed the utils branch from bb99920 to 1ace0f8 Compare July 12, 2021 21:02

mkroening force-pushed the utils branch 2 times, most recently from 1a47dfc to 2a412a4 Compare July 14, 2021 12:11

mkroening force-pushed the utils branch from 2a412a4 to 008edcb Compare July 23, 2021 16:17

mkroening changed the title ~~[DO NOT MERGE] Rework utils module~~ [DO NOT MERGE] Split up utils module Jul 23, 2021

mkroening force-pushed the utils branch 2 times, most recently from 558f88e to dab5aec Compare July 28, 2021 13:19

mkroening changed the title ~~[DO NOT MERGE] Split up utils module~~ Split up utils module Jul 28, 2021

mkroening requested a review from jounathaen July 28, 2021 13:20

mkroening marked this pull request as ready for review July 28, 2021 13:20

jounathaen reviewed Jul 28, 2021

View reviewed changes

src/bin_utils.rs Outdated Show resolved Hide resolved

mkroening force-pushed the utils branch from dab5aec to 619bd57 Compare July 28, 2021 13:32

mkroening requested a review from stlankes August 9, 2021 08:41

mkroening force-pushed the utils branch from 619bd57 to 865daf3 Compare August 20, 2021 14:27

bors bot added a commit that referenced this pull request Aug 20, 2021

Merge #138

656a655

138: Split up utils module r=jounathaen a=mkroening Binaries get a much smaller bin_utils and other helper functions are replaced/moved. Co-authored-by: Martin Kröning <mkroening@posteo.net>

mkroening added 2 commits August 20, 2021 16:28

Split up utils module

ad88268

Binaries get a much smaller bin_utils and other helper functions are replaced/moved.

Coverage: Disable doctest verification

6715a30

mkroening force-pushed the utils branch from 865daf3 to 6715a30 Compare August 20, 2021 14:28

bors bot added a commit that referenced this pull request Aug 20, 2021

Merge #138

e4f0da4

138: Split up utils module r=jounathaen a=mkroening Binaries get a much smaller bin_utils and other helper functions are replaced/moved. Co-authored-by: Martin Kröning <mkroening@posteo.net>

bors bot added a commit that referenced this pull request Aug 20, 2021

Merge #138

8512bb7

138: Split up utils module r=jounathaen a=mkroening Binaries get a much smaller bin_utils and other helper functions are replaced/moved. Co-authored-by: Martin Kröning <mkroening@posteo.net>

bors bot merged commit 956dbcd into hermit-os:master Aug 21, 2021

mkroening deleted the utils branch August 21, 2021 12:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split up utils module #138

Split up utils module #138

mkroening commented Jul 11, 2021 •

edited

mkroening commented Jul 12, 2021

codecov bot commented Jul 12, 2021 •

edited

mkroening commented Jul 28, 2021

jounathaen Jul 28, 2021

jounathaen Jul 28, 2021

mkroening Jul 28, 2021

jounathaen commented Jul 28, 2021

mkroening commented Jul 28, 2021

jounathaen commented Jul 28, 2021

mkroening commented Jul 28, 2021

jounathaen commented Aug 20, 2021

bors bot commented Aug 20, 2021

jounathaen commented Aug 20, 2021

bors bot commented Aug 20, 2021

jounathaen commented Aug 20, 2021

bors bot commented Aug 20, 2021

mkroening commented Aug 21, 2021

bors bot commented Aug 21, 2021

Split up utils module #138

Split up utils module #138

Conversation

mkroening commented Jul 11, 2021 • edited

mkroening commented Jul 12, 2021

codecov bot commented Jul 12, 2021 • edited

Codecov Report

mkroening commented Jul 28, 2021

jounathaen Jul 28, 2021

Choose a reason for hiding this comment

jounathaen Jul 28, 2021

Choose a reason for hiding this comment

mkroening Jul 28, 2021

Choose a reason for hiding this comment

jounathaen commented Jul 28, 2021

mkroening commented Jul 28, 2021

jounathaen commented Jul 28, 2021

mkroening commented Jul 28, 2021

jounathaen commented Aug 20, 2021

bors bot commented Aug 20, 2021

jounathaen commented Aug 20, 2021

bors bot commented Aug 20, 2021

jounathaen commented Aug 20, 2021

bors bot commented Aug 20, 2021

mkroening commented Aug 21, 2021

bors bot commented Aug 21, 2021

mkroening commented Jul 11, 2021 •

edited

codecov bot commented Jul 12, 2021 •

edited