Add systemwide config file support #1888

patrickpichler · 2021-10-06T18:37:59Z

There is now support for a systemwide config file. The location of the
system wide config file is $(BAT_SYSTEM_CONFIG_PREFIX)/bat/config.
$(BAT_SYSTEM_CONFIG_PREFIX) has to be provided at compile time as an
environment variable. If the environment variable is not set, a default
is used. This default is C:\ProgramData for windows and /etc for every other os.

fixes #668

sharkdp · 2021-10-24T10:22:11Z

@patrickpichler Thank you very much. This looks good to me, except for one minor inline comment. Can we please add a test for this?

For CI, I guess we could either create a file in /etc, or provide the environment variable at compile time (and set it to some folder inside tests/). However, since we also want to run tests locally, we should probably follow the second path.

src/bin/bat/config.rs

patrickpichler · 2021-10-25T11:06:08Z

@sharkdp I like the idea with the test, but I'm not 100% sure how to add it.
How would I set the environment variable needed at build time only for the
tests?

sharkdp · 2021-10-26T06:31:48Z

@sharkdp I like the idea with the test, but I'm not 100% sure how to add it.
How would I set the environment variable needed at build time only for the
tests?

Hm.. I didn't really think about it. Maybe that's not possible, you're right. I thought it might work to just build normally but then test with BAT_SYSTEM_CONFIG_PREFIX=… cargo test. But it's likely this is not going to work.

I'd be okay with a CI-only test where it would be fine to set BAT_SYSTEM_CONFIG_PREFIX globally. At run time (i.e. unit test run time), we could then check if system_config_file() points to a predetermined folder in tests and only run the test conditionally in that situation.

I realize this doesn't sound too appealing, so I'm also glad to hear other ideas.

src/bin/bat/main.rs

patrickpichler · 2021-10-26T08:20:44Z

I'd be okay with a CI-only test

If i understand the CICD.yml correctly, I would need to build the application then
twice, as otherwise a wrong BAT_SYSTEM_CONFIG_PREFIX would be published
to the Github release page.

I realize this doesn't sound too appealing, so I'm also glad to hear other ideas.

One idea would be to do something like git is doing. They allow to override the location
of the system and global config files via an environment variable. This way I could at
least test that the logic in general is working.

sharkdp · 2021-10-26T08:34:31Z

I'd be okay with a CI-only test

If i understand the CICD.yml correctly, I would need to build the application then twice, as otherwise a wrong BAT_SYSTEM_CONFIG_PREFIX would be published to the Github release page.

Oh 🙈. You are absolutely right.

One idea would be to do something like git is doing. They allow to override the location
of the system and global config files via an environment variable. This way I could at
least test that the logic in general is working.

I think I would now prefer a custom test stage in the CI script where we explicitly place a system config file in /etc and then call a special unit test that is disabled/ignored by default. We do something similar in the "Run ignored-by-default unit tests with new syntaxes and themes" stage.

I hope there is no clash with the existing #[ignore]d unit tests. If there is, or if the configuration get's too complicated, we could also think of a integration-style test that's implemented in the CICD.yml file (i.e. in bash).

I realize that it sounds like a lot of effort for a minor piece of code. But we really want to make sure that we have a solid testing framework in order to reduce the maintenance work in the future. Having a solid test for this will ensure that this feature continues to work in the future.

That said, if it turns out that maintaining the test code for this could result in major maintenance work in the future, I'm also okay with merging this without a test.

patrickpichler · 2021-10-26T08:59:11Z

I hope there is no clash with the existing #[ignore]d unit tests

Mhm I guess this will cause issues in the Run ignored-by-default unit tests with new syntaxes and themes step.

I could change it to explicitly provide the --test argument to cargo test, so that only the tests
in tests/assets.rs are executed. Then add a new integration test file for the global config, which
is also #[ignore]d by default and have an explicit step in the CICD.yml to run the tests.

How does this sound?

sharkdp · 2021-10-26T19:29:56Z

Sounds great!

patrickpichler · 2021-10-28T13:31:25Z

@sharkdp I've added some tests, plus made the required changes in the last commit 👍

patrickpichler · 2021-11-10T17:49:50Z

@sharkdp any update on this?

sharkdp · 2021-11-22T19:02:11Z

tests/system_wide_config.rs

+fn bat_raw_command() -> Command {
+    let mut cmd = Command::cargo_bin("bat").unwrap();
+    cmd.current_dir("tests/examples");
+    cmd.env_remove("BAT_CACHE_PATH");
+    cmd.env_remove("BAT_CONFIG_DIR");
+    cmd.env_remove("BAT_CONFIG_PATH");
+    cmd.env_remove("BAT_OPTS");
+    cmd.env_remove("BAT_PAGER");
+    cmd.env_remove("BAT_STYLE");
+    cmd.env_remove("BAT_TABS");
+    cmd.env_remove("BAT_THEME");
+    cmd.env_remove("COLORTERM");
+    cmd.env_remove("NO_COLOR");
+    cmd.env_remove("PAGER");
+    cmd
+}
+
+fn bat() -> assert_cmd::Command {
+    assert_cmd::Command::from_std(bat_raw_command())
+}


it would be great to move this duplicated(?) code to a common test-helper module

sharkdp · 2021-11-22T19:02:42Z

tests/system_wide_config.rs

+    assert_cmd::Command::from_std(bat_raw_command())
+}
+
+// This test is ignored, as it needs a special system wide config put into place


can we please add instruction on how to enable it? currently, it sounds like it is ignored for good :-). or maybe just add a reference to the CI config.

sharkdp · 2021-11-22T19:05:21Z

Thank you for the updates!

Could you please add an entry to the CHANGELOG? See instructions here: https://github.com/sharkdp/bat/blob/master/CONTRIBUTING.md#add-an-entry-to-the-changelog

patrickpichler · 2021-11-28T09:52:56Z

@sharkdp I've added the CHANGELOG.md entry, plus added a short description of the feature to the README.md

sharkdp

Thank you

sharkdp · 2021-11-28T10:38:05Z

Tests are currently failing

tests/integration_tests.rs

sharkdp · 2021-11-28T11:31:53Z

tests/utils/command.rs

+    cmd
+}
+
+pub fn bat_raw_command() -> Command {


I think this function (or the whole module) should be #[cfg(test)], to silence the warnings. Sorry, just saw this now.

Similar for the other functions in this file.

I've tried putting #[cfg(test)] on there, but cargo test still emits the warning when building.
Or did you mean some other warnings?

After looking at it again, I guess rust still emits a warning, as the function is only
used in unix tests. I'm not 100% sure how to fix this though, as one cannot put multiple
cfg annotations on top. Do you maybe have an idea?

#[cfg(test)] is not enough (but does not hurt), because the mod is brought into scope by a test, but then never used. There are ways to restructure the code, but I don't think it is worth it, so let's just allow it, at least for now.

There is now support for a systemwide config file. The location of the system wide config file is `$(BAT_SYSTEM_CONFIG_PREFIX)/bat/config`. `$(BAT_SYSTEM_CONFIG_PREFIX)` has to be provided at compile time as an environment variable. If the environment variable is not set, a default is used. This default is `C:\ProgramData` for windows and `/etc` for every other os.

There is now a new stage in the CICD workflow present, which will build `bat` with the `BAT_SYSTEM_CONFIG_PREFIX` set to load the config file from `/tests/examples/system_config/bat/config`, plus a basic set of tests, to ensure the feature is working as expected. By default the tests are set to ignored, as they need special setup before they can be run.

patrickpichler · 2022-03-10T20:27:02Z

@sharkdp @Enselic any update on this?
I looked once again into the warnings on running the system wide tests and I guess this
is caused as code is shared between the different test types.

The warnings also show up for other shared utility methods such as the one defined in
tests/utils/mocked_pagers.rs.

Do you have any plan what I could do here?

…temwide-config

Enselic · 2022-09-04T19:51:13Z

Looks like I got rid of the warnings now, and this looks like a nice
change to me. It has tests, CI is green, and @sharkdp even approved it
already once. Sorry for taking so long, @patrickpichler.

@sharkdp Any chance you can take a final look and then merge? I
haven't done a detailed review yet, but if you think it look good, I'm
sure it's good enough. And even if it is not perfect (what is,
really?) it certainly seems good enough to start off with and then
iteratively improve on over several versions of bat.

sharkdp · 2022-09-04T20:11:48Z

I did have another look and fully agree. Thank you @patrickpichler and @Enselic for the update.

patrickpichler force-pushed the feature/668/add-systemwide-config branch 3 times, most recently from 818e044 to 6617650 Compare October 6, 2021 18:57

sharkdp reviewed Oct 24, 2021

View reviewed changes

src/bin/bat/config.rs Show resolved Hide resolved

patrickpichler force-pushed the feature/668/add-systemwide-config branch from 6617650 to 1f47b80 Compare October 25, 2021 11:01

sharkdp reviewed Oct 26, 2021

View reviewed changes

src/bin/bat/main.rs Outdated Show resolved Hide resolved

patrickpichler force-pushed the feature/668/add-systemwide-config branch from 1f47b80 to a2e937c Compare October 26, 2021 08:17

patrickpichler force-pushed the feature/668/add-systemwide-config branch from 25f6f03 to fccb57d Compare October 28, 2021 13:49

patrickpichler marked this pull request as ready for review October 28, 2021 14:08

sharkdp reviewed Nov 22, 2021

View reviewed changes

sharkdp added this to the v0.19 milestone Nov 22, 2021

sharkdp approved these changes Nov 28, 2021

View reviewed changes

patrickpichler force-pushed the feature/668/add-systemwide-config branch from 878db6b to 6d69225 Compare November 28, 2021 10:48

sharkdp reviewed Nov 28, 2021

View reviewed changes

tests/integration_tests.rs Outdated Show resolved Hide resolved

sharkdp reviewed Nov 28, 2021

View reviewed changes

patrickpichler force-pushed the feature/668/add-systemwide-config branch 2 times, most recently from 67d354f to 1ce36e6 Compare January 8, 2022 17:52

Enselic added the needs-work label Feb 8, 2022

Enselic removed this from the v0.19 milestone Mar 3, 2022

Patrick Pichler added 2 commits March 10, 2022 20:55

patrickpichler force-pushed the feature/668/add-systemwide-config branch from 1ce36e6 to 6b660ef Compare March 10, 2022 20:27

Enselic added 3 commits September 4, 2022 20:44

Merge remote-tracking branch 'origin/master' into feature/668/add-sys…

2dd2994

…temwide-config

Move up the entry to the unreleased section in CHANGELOG.md

bc2dc07

Try to fix all warnings

799bb16

Enselic removed the needs-work label Sep 4, 2022

sharkdp merged commit 546dcf6 into sharkdp:master Sep 4, 2022

crazygolem mentioned this pull request May 2, 2023

Separate system and user custom assets #2559

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add systemwide config file support #1888

Add systemwide config file support #1888

patrickpichler commented Oct 6, 2021 •

edited

Loading

sharkdp commented Oct 24, 2021

patrickpichler commented Oct 25, 2021

sharkdp commented Oct 26, 2021

patrickpichler commented Oct 26, 2021

sharkdp commented Oct 26, 2021 •

edited

Loading

patrickpichler commented Oct 26, 2021

sharkdp commented Oct 26, 2021

patrickpichler commented Oct 28, 2021

patrickpichler commented Nov 10, 2021

sharkdp Nov 22, 2021

sharkdp Nov 22, 2021 •

edited

Loading

sharkdp commented Nov 22, 2021 •

edited

Loading

patrickpichler commented Nov 28, 2021

sharkdp left a comment

sharkdp commented Nov 28, 2021

sharkdp Nov 28, 2021

patrickpichler Nov 28, 2021

patrickpichler Jan 11, 2022

Enselic Sep 4, 2022

patrickpichler commented Mar 10, 2022

Enselic commented Sep 4, 2022

sharkdp commented Sep 4, 2022

Add systemwide config file support #1888

Add systemwide config file support #1888

Conversation

patrickpichler commented Oct 6, 2021 • edited Loading

sharkdp commented Oct 24, 2021

patrickpichler commented Oct 25, 2021

sharkdp commented Oct 26, 2021

patrickpichler commented Oct 26, 2021

sharkdp commented Oct 26, 2021 • edited Loading

patrickpichler commented Oct 26, 2021

sharkdp commented Oct 26, 2021

patrickpichler commented Oct 28, 2021

patrickpichler commented Nov 10, 2021

sharkdp Nov 22, 2021

Choose a reason for hiding this comment

sharkdp Nov 22, 2021 • edited Loading

Choose a reason for hiding this comment

sharkdp commented Nov 22, 2021 • edited Loading

patrickpichler commented Nov 28, 2021

sharkdp left a comment

Choose a reason for hiding this comment

sharkdp commented Nov 28, 2021

sharkdp Nov 28, 2021

Choose a reason for hiding this comment

patrickpichler Nov 28, 2021

Choose a reason for hiding this comment

patrickpichler Jan 11, 2022

Choose a reason for hiding this comment

Enselic Sep 4, 2022

Choose a reason for hiding this comment

patrickpichler commented Mar 10, 2022

Enselic commented Sep 4, 2022

sharkdp commented Sep 4, 2022

patrickpichler commented Oct 6, 2021 •

edited

Loading

sharkdp commented Oct 26, 2021 •

edited

Loading

sharkdp Nov 22, 2021 •

edited

Loading

sharkdp commented Nov 22, 2021 •

edited

Loading