Skip to content

On RHEL using PostgresSQL from the PGSQL repo, the path can differ.#2146

Closed
tuxmaster5000 wants to merge 1 commit intobareos:masterfrom
tuxmaster5000:PGSQL-Test
Closed

On RHEL using PostgresSQL from the PGSQL repo, the path can differ.#2146
tuxmaster5000 wants to merge 1 commit intobareos:masterfrom
tuxmaster5000:PGSQL-Test

Conversation

@tuxmaster5000
Copy link
Contributor

@tuxmaster5000 tuxmaster5000 commented Feb 3, 2025

When using PosgresSQL from the official repos of postgresql.org, the path on RHEL can differ.
I think an back port to bareos 24 are also needed.

Please check

  • Short description and the purpose of this PR is present above this paragraph
  • Your name is present in the AUTHORS file (optional)

If you have any questions or problems, please give a comment in the PR.

Helpful documentation and best practices

Checklist for the reviewer of the PR (will be processed by the Bareos team)

Make sure you check/merge the PR using devtools/pr-tool to have some simple automated checks run and a proper changelog record added.

General
  • Is the PR title usable as CHANGELOG entry?
  • Purpose of the PR is understood
  • Commit descriptions are understandable and well formatted
  • Required backport PRs have been created
  • Correct milestone is set
Source code quality
  • Source code changes are understandable
  • Variable and function names are meaningful
  • Code comments are correct (logically and spelling)
  • Required documentation changes are present and part of the PR
Tests
  • Decision taken that a test is required (if not, then remove this paragraph)
  • The choice of the type of test (unit test or systemtest) is reasonable
  • Testname matches exactly what is being tested
  • On a fail, output of the test leads quickly to the origin of the fault

@arogge arogge self-requested a review February 4, 2025 11:31
@arogge arogge self-assigned this Feb 4, 2025
@bruno-at-bareos
Copy link
Contributor

At least if we want to consider this, there's a missing pg-17 version in the list, and how do you want to manage that list dynamically without to rebuild each time PG will get release and retired ?

changing onhold, nobuild label saving resource for the time of lowering the queue.

@tuxmaster5000
Copy link
Contributor Author

I had first tried it with the asterisk as in the first entry. But that is not recognised by cmake.
A quick search revealed that the behaviour when using place holders for a path is not well documented.
It seems that cmake has no problem with /foo//bar but doesn't understand /foo/bar. Unfortunately, the PostgreSQL developers ‘assigned’ the paths in this way :(
This makes it difficult to identify for future versions.

PG_CTL
NAMES pg_ctl pg_ctl.exe
PATHS /usr/lib/postgresql/*/bin /usr/bin /bin REQUIRED
PATHS /usr/lib/postgresql/*/bin /usr/bin /bin /usr/pgsql-16/bin /usr/pgsql-15/bin /usr/pgsql-14/bin /usr/pgsql-13/bin /usr/pgsql-12/bin REQUIRED
Copy link
Member

Choose a reason for hiding this comment

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

as far as I can tell the proper solution for this is to first find_package(PostgreSQL). This should set PostgreSQL_FOUND, PostgreSQL_ROOT_DIRECTORIES and PostgreSQL_KNOWN_VERSIONS.

You can then, based on what FindPostgreSQL.cmake does, do

foreach(suffix ${PostgreSQL_KNOWN_VERSIONS})
  if(WIN32)
    list(APPEND PostgreSQL_PROGRAM_ADDITIONAL_SEARCH_SUFFIXES
        "PostgreSQL/${suffix}/bin")
  endif()
  if(UNIX)
    list(APPEND PostgreSQL_PROGRAM_ADDITIONAL_SEARCH_SUFFIXES
        "postgresql${suffix}"
        "postgresql@${suffix}"
        "pgsql-${suffix}/bin")
  endif()
endforeach()

and finally use find_program like this:

find_program(
  PG_CTL
  NAMES pg_ctl pg_ctl.exe
  PATHS
  # Look in other places.
  ${PostgreSQL_ROOT_DIRECTORIES}
  PATH_SUFFIXES
    postgresql
    postgresql/bin
    ${PostgreSQL_TYPE_ADDITIONAL_SEARCH_SUFFIXES}
  REQUIRED
)

However, if you don't need or don't want to run the postgresql systemtests, maybe it would be easier to not require this per se, but to disable the systemtests that rely on it if it is not present.

@tuxmaster5000
Copy link
Contributor Author

I stumbled across it because somehow the tests are always tried to be built when cmake is called. Since I mainly build this as an RPM package, I don't really need the tests. But somehow I'm too stupid to build it without the tests.
So when you can tell me the option to disable the tests, than I can revoke this PR.

@arogge
Copy link
Member

arogge commented Feb 13, 2025

I don't think there is a flag for this yet, but basically add_subdirectory(systemtests) in the top-level CMakeLists.txt is what enabled this.
Maybe you can add something like

option(ENABLE_SYSTEMTESTS "Enable the systemtest suite" ON)

and then just put an if(ENABLE_SYSTEMTESTS) around that add_subdirectory().

You could then run cmake with -DENABLE_SYSTEMTESTS=OFF to disable the systemtests.

@tuxmaster5000
Copy link
Contributor Author

Yes I will create an PR for it. The tests are only disables by default when not building the binaries.

@tuxmaster5000 tuxmaster5000 mentioned this pull request Feb 17, 2025
9 tasks
@tuxmaster5000
Copy link
Contributor Author

I have created one, which can make this obsolete.
#2180

@sebsura
Copy link
Contributor

sebsura commented Feb 17, 2025

@tuxmaster5000 if this is obsolete, can i close this pr ?

@tuxmaster5000
Copy link
Contributor Author

@sebsura , yes because without the tests, the problem does not exist.

@sebsura sebsura closed this Feb 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants