[#73] Adds build.rs to generate a default config template with tool version #78

MitchellBerend · 2022-09-10T14:27:25Z

Resolves #73
This adds a build script that generates the src/config/template.rs file. It is
done this way because there is no access to version information after compile
time. It uses the CARGO_PKG_VERSION environment variable made available for
cargo after running the command cargo build.

Additional tasks

tasks following #73

Automatically embedding version. As discussed in this issue.
Prefill all the tools supported by tool-sync. This requires converting the
big match to a BTreeMap.
Add comments around tool tables. To make sure that the code inside the
comments stays up-to-date and is parsed by tool-sync itself.

General tasks

Documentation for changes provided/changed
Tests added

build.rs

chshersh

Thanks for attempting to do this!

It's an interesting approach 🤔 But I tend to think that we don't need compile-time generation in this particular case.

I imagine using the env! macro to read the value of the CARGO_PKG_VERSION env variable and format! inside the src/config/template.rs to produce a final default configuration.

It'll become uglier than having just a plain hardcoded string but I can live with a bit of ugliness 👍🏻 The existing approach won't work if we want to fill all the hardcoded tools from a BTreeMap. To do this with build.rs, we had to move this map here and the implementation will become more convoluted.

I do, however, see the benefit in having the full default config written as plain text. So I propose to use golden testing here. Let's create the tests/default.toml file with the hardcoded config as it will be generated. And add a test that reads the file and compares it with the result of generating the config.

This is known as the golden testing technique. It's nice to have the full text to visibly see the content. The important part here is that we also test this text against the result of generation to make sure that we do generate the valid config.

chshersh

Looks great! 👍🏻

Just a few comments 🙂

src/sync/db.rs

src/config/template.rs

… characters

MitchellBerend · 2022-09-11T10:48:53Z

I must have clicked a ready button or something while reading some comments on this pr. It still needs a lot of work and I would also like to add a doc tests for the output of the default-config command

MitchellBerend · 2022-09-11T15:19:30Z

Im adding some more tests to increase coverage. Im using cargo tarpaulin to check this.
There are still some false negatives in tarpaulin but adding more tests is probably not a bad thing.

I have found that this method was not testable since it exits the program:

tool-sync/src/config/schema.rs

Lines 45 to 63 in 4341cc7

    
           pub fn ensure_store_directory(&self) -> PathBuf { 
        
               let expanded_store_directory = shellexpand::full(&self.store_directory); 
        
               let store_directory = match expanded_store_directory { 
        
                   Err(e) => err::abort_with(&e.to_string()), 
        
                   Ok(cow_path) => PathBuf::from(cow_path.into_owned()), 
        
               }; 
        
               let has_store_directory = store_directory.as_path().is_dir(); 
        
               if !has_store_directory { 
        
                   err::abort_with(&format!( 
        
                       "Specified directory for storing tools doesn't exist: {}", 
        
                       store_directory.display() 
        
                   )); 
        
               } 
        
               store_directory 
        
           }

main brach

Sep 11 17:10:36.148  INFO cargo_tarpaulin::process_handling::linux: Launching test
Sep 11 17:10:36.148  INFO cargo_tarpaulin::process_handling: running /home/mitchell/rust/tool-sync/target/debug/deps/tool-711d73e056f99e07

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

Sep 11 17:10:38.071  INFO cargo_tarpaulin::process_handling::linux: Launching test
Sep 11 17:10:38.071  INFO cargo_tarpaulin::process_handling: running /home/mitchell/rust/tool-sync/target/debug/deps/tool_sync-fd5c4dc2ba40d47f

running 21 tests
test config::toml::tests::empty_file ... ok
test config::toml::tests::store_directory_is_a_number ... ok
test config::toml::tests::only_store_directory ... ok
test config::toml::tests::single_empty_tool ... ok
test config::toml::tests::single_partial_tool ... ok
test config::toml::tests::single_full_tool ... ok
test config::toml::tests::store_directory_is_dotted ... ok
test config::toml::tests::two_empty_tools ... ok
test model::asset_name::tests::exe_name ... ok
test model::asset_name::tests::asset_name ... ok
test sync::configure::tests::full_override ... ok
test sync::configure::tests::full_configuration ... ok
test sync::configure::tests::known_tool_with_empty_config_asset ... ok
test sync::configure::tests::unknown_tool_with_empty_config_asset ... ok
test sync::configure::tests::partial_configuration ... ok
test sync::configure::tests::wrong_tool_with_empty_config_asset ... ok
test sync::download::tests::release_url_with_latest_tag_is_correct ... ok
test sync::configure::tests::partial_override ... ok
test sync::download::tests::release_url_with_specific_tag_is_correct ... ok
test sync::progress::tests::test_max_tag_size_specific ... ok
test sync::progress::tests::test_max_tag_size_latest ... ok

test result: ok. 21 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.05s

Sep 11 17:10:40.028  INFO cargo_tarpaulin::report: Coverage Results:
|| Uncovered Lines:
|| src/config/schema.rs: 45-46, 48-50, 53, 55-56, 58
|| src/config/toml.rs: 17-21, 26-27, 29
|| src/err.rs: 4-5, 12, 16-17, 30
|| src/lib.rs: 16-18, 20-26, 29-30, 33, 37-45, 47-48, 54-55
|| src/main.rs: 2
|| src/model/tool.rs: 19-22, 24
|| src/sync/archive.rs: 28-33, 40, 46, 48-53, 55-56, 58-63, 65-66, 68-72, 81-82, 84, 87-89, 93-95, 101, 103-106, 109-110, 112, 114, 117, 123, 126, 128-130, 133-134, 138, 140, 145-146, 148-151
|| src/sync/configure.rs: 22
|| src/sync/db.rs: 8-14, 16, 19-25, 27, 30-36, 38, 41-47, 49, 63-69, 71
|| src/sync/download.rs: 31-33, 37-38, 40-42, 46-47, 50-52, 55, 57, 60-61, 64-66, 69, 71-72, 74-75, 77-80, 83-84, 87-88, 90, 94-95, 97, 99-100, 102, 104-107, 109-112, 119-122
|| src/sync/install.rs: 29-33, 43-45, 47-50, 52-54, 57-59, 64, 69, 71, 73, 75-78, 83, 87-89, 92, 94, 96-100, 109-110, 112-114, 117, 119, 121, 128-129
|| src/sync/progress.rs: 38-39, 47, 50-51, 53-56, 60-61, 64-65, 68-69, 72-73, 75-77, 80-81, 83-85
|| src/sync.rs: 12-14, 32, 34-35, 38, 40-41, 43-44
|| Tested/Total Lines:
|| src/config/schema.rs: 0/9 +0.00%
|| src/config/toml.rs: 71/79 -10.13%
|| src/err.rs: 0/6 +0.00%
|| src/lib.rs: 0/26 +0.00%
|| src/main.rs: 1/2 +0.00%
|| src/model/asset_name.rs: 16/16 +0.00%
|| src/model/tool.rs: 4/9 +0.00%
|| src/sync/archive.rs: 0/63 +0.00%
|| src/sync/configure.rs: 108/109 +0.00%
|| src/sync/db.rs: 16/56 -71.43%
|| src/sync/download.rs: 14/66 +0.00%
|| src/sync/install.rs: 0/47 +0.00%
|| src/sync/progress.rs: 29/54 +0.00%
|| src/sync.rs: 0/11 +0.00%
|| 
46.84% coverage, 259/553 lines covered, -12.641681145031853% change in coverage

This branch

Sep 11 17:15:10.011  INFO cargo_tarpaulin::process_handling::linux: Launching test
Sep 11 17:15:10.011  INFO cargo_tarpaulin::process_handling: running /home/mitchell/rust/tool-sync/target/debug/deps/tool-711d73e056f99e07

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

Sep 11 17:15:11.899  INFO cargo_tarpaulin::process_handling::linux: Launching test
Sep 11 17:15:11.899  INFO cargo_tarpaulin::process_handling: running /home/mitchell/rust/tool-sync/target/debug/deps/tool_sync-fd5c4dc2ba40d47f

running 27 tests
test config::toml::tests::store_directory_is_a_number ... ok
test config::toml::tests::empty_file ... ok
test config::toml::tests::only_store_directory ... ok
test config::toml::tests::single_empty_tool ... ok
test config::toml::tests::single_partial_tool ... ok
test config::toml::tests::single_full_tool ... ok
test config::toml::tests::test_toml_error_display_decode ... ok
test config::toml::tests::test_toml_error_display_io ... ok
test config::toml::tests::test_parse_file_error ... ok
test config::toml::tests::test_parse_file_correct_output ... ok
test config::toml::tests::store_directory_is_dotted ... ok
test config::toml::tests::test_parse_file_passing_tools_read ... ok
test config::toml::tests::two_empty_tools ... ok
test model::asset_name::tests::exe_name ... ok
test config::toml::tests::test_toml_error_display_parse ... ok
test model::asset_name::tests::asset_name ... ok
test sync::configure::tests::unknown_tool_with_empty_config_asset ... ok
test sync::configure::tests::partial_configuration ... ok
test sync::configure::tests::known_tool_with_empty_config_asset ... ok
test sync::configure::tests::partial_override ... ok
test sync::configure::tests::full_override ... ok
test sync::configure::tests::full_configuration ... ok
test sync::configure::tests::wrong_tool_with_empty_config_asset ... ok
test sync::progress::tests::test_max_tag_size_specific ... ok
test sync::progress::tests::test_max_tag_size_latest ... ok
test sync::download::tests::release_url_with_specific_tag_is_correct ... ok
test sync::download::tests::release_url_with_latest_tag_is_correct ... ok

test result: ok. 27 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.05s

Sep 11 17:15:13.851  INFO cargo_tarpaulin::report: Coverage Results:
|| Uncovered Lines:
|| src/config/schema.rs: 45-46, 48-50, 53, 55-56, 58
|| src/config/template.rs: 3-4
|| src/err.rs: 4-5, 12, 16-17, 30
|| src/lib.rs: 16-18, 20-26, 29-30, 33, 37-45, 47-48, 54-55
|| src/main.rs: 2
|| src/model/tool.rs: 19-22, 24
|| src/sync/archive.rs: 28-33, 40, 46, 48-53, 55-56, 58-63, 65-66, 68-72, 81-82, 84, 87-89, 93-95, 101, 103-106, 109-110, 112, 114, 117, 123, 126, 128-130, 133-134, 138, 140, 145-146, 148-151
|| src/sync/configure.rs: 22
|| src/sync/download.rs: 31-33, 37-38, 40-42, 46-47, 50-52, 55, 57, 60-61, 64-66, 69, 71-72, 74-75, 77-80, 83-84, 87-88, 90, 94-95, 97, 99-100, 102, 104-107, 109-112, 119-122
|| src/sync/install.rs: 29-33, 43-45, 47-50, 52-54, 57-59, 64, 69, 71, 73, 75-78, 83, 87-89, 92, 94, 96-100, 109-110, 112-114, 117, 119, 121, 128-129
|| src/sync/progress.rs: 38-39, 47, 50-51, 53-56, 60-61, 64-65, 68-69, 72-73, 75-77, 80-81, 83-85
|| src/sync.rs: 12-14, 32, 34-35, 38, 40-41, 43-44
|| Tested/Total Lines:
|| src/config/schema.rs: 0/9 +0.00%
|| src/config/template.rs: 0/2
|| src/config/toml.rs: 117/117 +10.13%
|| src/err.rs: 0/6 +0.00%
|| src/lib.rs: 0/26 +0.00%
|| src/main.rs: 1/2 +0.00%
|| src/model/asset_name.rs: 16/16 +0.00%
|| src/model/tool.rs: 4/9 +0.00%
|| src/sync/archive.rs: 0/63 +0.00%
|| src/sync/configure.rs: 108/109 +0.00%
|| src/sync/db.rs: 73/73 +71.43%
|| src/sync/download.rs: 14/66 +0.00%
|| src/sync/install.rs: 0/47 +0.00%
|| src/sync/progress.rs: 29/54 +0.00%
|| src/sync.rs: 0/11 +0.00%
|| 
59.34% coverage, 362/610 lines covered, +12.508819257107277% change in coverage

MitchellBerend · 2022-09-12T10:32:17Z

@chshersh Should the default tools also be generated in the default-config command? This would make it so maintenance only needs to happen in one place (the BTreeMap).

chshersh · 2022-09-12T12:20:49Z

@MitchellBerend cargo tarpaulin looks like a nice tool 🙂 I wouldn't try to gamify code coverage too much. Achieving 100% actually might be counterproductive as your test freeze your code too much and you need to do more and more to change the tests each time you change the code. At this stage, tool-sync is quite unstable and lots of refactorings could happen by changing the codebase significantly.

It's useful however to identify important code paths that are not covered 👍🏻

Should the default tools also be generated in the default-config command? This would make it so maintenance only needs to happen in one place (the BTreeMap).

Yes, that was the plan. If you're up to it, you can go ahead and do this change in this PR (or leave it for later).

… always includes all tools defined in src/sync/db.rs

src/config/template.rs

MitchellBerend · 2022-09-13T17:37:01Z

I think this is ready for review. I would like to have more tests still though but I agree with the not having too high of a code coverage. The tests I added were to confirm the formatting of printing errors when processing toml files so I would like those in.

chshersh

Great work, as always! 🥇

It's nice to see the conversion to BTreeMap of all the tools, it opens new possibilities 🙂

I have a few comments and some clarification questions but overall it looks really great!

.github/workflows/ci.yml

src/config/template.rs

src/sync/db.rs

src/config/toml.rs

…d fail

…he parsing fails before it asserts

.github/workflows/ci.yml

Co-authored-by: Dmitrii Kovanikov <[email protected]>

chshersh

I believe it's good to go 🚢
Thanks a lot for your huge work, @MitchellBerend 🏆

Nothing is immutable so the code always can be improved later in case we have more ideas 🙂
But this PR is really helpful.

MitchellBerend added 5 commits September 10, 2022 16:12

adding cargo fmted files

e4a281f

Added comment back that was deleted accidentally

56910b3

fixed formatting

460d021

fixed more formatting issues

3e5d4f7

fixed yet more formatting issues

f7f5c57

MitchellBerend added documentation Improvements or additions to documentation help wanted Extra attention is needed config TOML configuration, config-related CLI options output Fancy (and not so) output of the tool labels Sep 10, 2022

MitchellBerend commented Sep 10, 2022

View reviewed changes

build.rs Outdated Show resolved Hide resolved

MitchellBerend commented Sep 10, 2022

View reviewed changes

build.rs Outdated Show resolved Hide resolved

fixed variable name

243eb56

chshersh requested changes Sep 10, 2022

View reviewed changes

MitchellBerend added 4 commits September 10, 2022 19:12

changed back template and incorporated a version

551706b

converted match statement to a hashmap

d5196ac

fixed unwrapping issue

ea0bd50

Merge branch 'main' into 73-add-version-to-config

4c11c31

MitchellBerend marked this pull request as ready for review September 10, 2022 20:34

chshersh requested changes Sep 11, 2022

View reviewed changes

src/sync/db.rs Show resolved Hide resolved

src/config/template.rs Outdated Show resolved Hide resolved

chshersh assigned MitchellBerend Sep 11, 2022

made the template a string literal so there is no need for escaping "…

3881ae7

… characters

MitchellBerend marked this pull request as draft September 11, 2022 10:43

MitchellBerend added 5 commits September 11, 2022 15:13

added default-config output test

4ffaae6

This commit should fail in ci

6e31ff8

corrected typo in test name and added more output when test fails

4b2320e

removed error to make ci pass again

10a5b70

added more tests to increase code coverage

62fef0e

MitchellBerend added 2 commits September 13, 2022 10:53

Merge branch 'main' into 73-add-version-to-config

ae60e3b

added a generated tools block so the default config that is generated…

8b12850

… always includes all tools defined in src/sync/db.rs

MitchellBerend commented Sep 13, 2022

View reviewed changes

src/config/template.rs Outdated Show resolved Hide resolved

converted simple panic to panic_suggest_issue from src/err.rs

093e667

MitchellBerend marked this pull request as ready for review September 13, 2022 17:35

chshersh requested changes Sep 13, 2022

View reviewed changes

MitchellBerend added 6 commits September 14, 2022 08:48

changed test only test the function itself instead of only the error

eaf6924

added more clarity on a test when its function succeeds when it shoul…

2870cb4

…d fail

removed test that is already covered in characterization test

c418edc

removed Option<ToolInfo> from the btree value type

0d7e91a

changed output of default-config command test

03cd372

wrapped parse_file in a catch unwind so the test actually checks if t…

6e1d5ed

…he parsing fails before it asserts

chshersh reviewed Sep 14, 2022

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

MitchellBerend and others added 2 commits September 14, 2022 18:13

Fixed typo

07c9d0e

Co-authored-by: Dmitrii Kovanikov <[email protected]>

fixed actual typo instead of removing it

9056e81

chshersh approved these changes Sep 15, 2022

View reviewed changes

chshersh merged commit b79eeb9 into chshersh:main Sep 15, 2022

MitchellBerend deleted the 73-add-version-to-config branch September 15, 2022 07:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#73] Adds build.rs to generate a default config template with tool version #78

[#73] Adds build.rs to generate a default config template with tool version #78

MitchellBerend commented Sep 10, 2022 •

edited

Loading

chshersh left a comment

chshersh left a comment

MitchellBerend commented Sep 11, 2022

MitchellBerend commented Sep 11, 2022

MitchellBerend commented Sep 12, 2022

chshersh commented Sep 12, 2022

MitchellBerend commented Sep 13, 2022

chshersh left a comment

chshersh left a comment

[#73] Adds build.rs to generate a default config template with tool version #78

[#73] Adds build.rs to generate a default config template with tool version #78

Conversation

MitchellBerend commented Sep 10, 2022 • edited Loading

Additional tasks

chshersh left a comment

Choose a reason for hiding this comment

chshersh left a comment

Choose a reason for hiding this comment

MitchellBerend commented Sep 11, 2022

MitchellBerend commented Sep 11, 2022

main brach

This branch

MitchellBerend commented Sep 12, 2022

chshersh commented Sep 12, 2022

MitchellBerend commented Sep 13, 2022

chshersh left a comment

Choose a reason for hiding this comment

chshersh left a comment

Choose a reason for hiding this comment

MitchellBerend commented Sep 10, 2022 •

edited

Loading