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

Improve handling of the default schema paths #980

Merged
merged 6 commits into from Jul 17, 2020

Conversation

tobim
Copy link
Member

@tobim tobim commented Jul 15, 2020

This PR removes the baking to the install prefix from the binary.

  • The path to the lowest priority schema files was already determined relative to the binary location at runtime. Now the help message properly reflects that.
  • The fallback path to vast.conf is now also determined by the runtime location.

This fixes 2 bugs:

  1. VAST_RELOCATABLE_INSTALL was not really relocatable because the paths above would not update on relocations.
  2. The CMAKE_INSTALL_PREFIX leaked into the static binary, which is relocatable by definition.

In addition, this PR also adds /etc/vast/schema to the schema file lookup path.

@tobim tobim added enhancement ✨ bug Incorrect behavior labels Jul 15, 2020
@tobim tobim requested a review from a team July 15, 2020 15:25
Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

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

Can you revert the change to the lookup order? PREFIX/etc/vast/vast.conf should come before /etc/vast/vast.conf in the ordering, and this PR changes that.

@tobim
Copy link
Member Author

tobim commented Jul 15, 2020

Can you revert the change to the lookup order? PREFIX/etc/vast/vast.conf should come before /etc/vast/vast.conf in the ordering, and this PR changes that.

That was intentional. As a user, I would intuitively edit /etc/vast/vast.conf and expect my changes to take effect, with the old ordering that expectation would not always be met. To me this is another small bump in the road that might negatively effect my evaluation of VAST.

@dominiklohmann
Copy link
Member

That was intentional. As a user, I would intuitively edit /etc/vast/vast.conf and expect my changes to take effect, with the old ordering that expectation would not always be met.

I don't get it. The usual behavior for programs is that the more specific a path to a configuration file, the earlier it should be in the lookup path. I think we should discuss this separately within the team (including ops) and have a vote on the expected and desired user experience.

As a side note: if this change was intentional, it should be reflected as such in the changelog.

@mavam
Copy link
Member

mavam commented Jul 15, 2020

The usual behavior for programs is that the more specific a path to a configuration file, the earlier it should be in the lookup path.

That's also my understanding. A home config file takes precedence over system-wide installed version. In that sense, /etc/vast is system and if the prefix is not /, it's more specific than the system.

@tobim
Copy link
Member Author

tobim commented Jul 16, 2020

I can only speak of what I'm used to and to some extent expect:

The configuration of a program is normally composed of 3 parts in order of precedence:

  1. The user level config, found in ~/.vast/vast.conf or ~/.config/vast/vast.conf (FSB scheme)
  2. The system level config in /etc/vast/vast.conf
  3. The defaults built into the application

What vast does at the moment:

  1. $PWD/vast.conf (A)
  2. <prefix>/share/vast/vast.conf (B)
  3. /etc/vast/vast.conf
  4. defaults

That's also my understanding. A home config file takes precedence over system-wide installed version. In that sense, /etc/vast is system and if the prefix is not /, it's more specific than the system.

This is not the way I think about the issue. <prefix>/share/vast/ contains settings defined by the upstream or vendor, thus they are the least specific. /etc/vast/ contains settings by the system administrator, thus they are more specific.

@tobim
Copy link
Member Author

tobim commented Jul 16, 2020

I think this change should be discussed in a separate thread though. I'm going to revert the change for this PR.

@tobim tobim force-pushed the story/ch18105/custom-install-prefix branch from d51a246 to a08c113 Compare July 16, 2020 15:51
CHANGELOG.md Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
libvast/src/system/application.cpp Show resolved Hide resolved
libvast/src/system/configuration.cpp Show resolved Hide resolved
vast.conf Outdated Show resolved Hide resolved
tobim and others added 2 commits July 17, 2020 10:08
Co-authored-by: Dominik Lohmann <mail@dominiklohmann.de>
Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

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

I verified in a Docker container that the additional schema paths get picked up.

@tobim tobim merged commit d2ba7fa into master Jul 17, 2020
@tobim tobim deleted the story/ch18105/custom-install-prefix branch July 17, 2020 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect behavior
Projects
None yet
3 participants