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

Automatically add Debian paths? #269

Open
tomjaguarpaw opened this issue Sep 9, 2020 · 12 comments
Open

Automatically add Debian paths? #269

tomjaguarpaw opened this issue Sep 9, 2020 · 12 comments

Comments

@tomjaguarpaw
Copy link
Collaborator

Using tmp-postgres with Debian is a bit awkward because initdb is not on the default PATH. What do you think about adding a feature that can add the paths at which initdb is typically found to the PATH? I've been using the following, but there are many possibilities.

-- Adds paths to PATH, runs the action and restores the original PATH
-- before returning.
withPath :: [String] -> IO a -> IO a
withPath paths f = bracket
  (do
      origPath <- System.Environment.lookupEnv "PATH"
      let newPath = maybe ps (++ ":" ++ ps) origPath
      System.Environment.setEnv "PATH" newPath
      pure origPath)
  (\origPath -> do
      let origPath' = maybe "" id origPath
      System.Environment.setEnv "PATH" origPath')
  (\_ -> f)

  where ps = intercalate ":" paths

-- withConfig, but adds to the PATH some typical locations that initdb
-- might be found (Debian doesn't put initdb on the PATH).
withConfigPath :: T.Config -> (T.DB -> IO a) -> IO (Either T.StartError a)
withConfigPath config f = withPath [ "/usr/lib/postgresql/11/bin"
                                   , "/usr/lib/postgresql/10/bin" ]
                                   (T.withConfig config f)
@jfischoff
Copy link
Owner

I tend to just put it on the PATH (like I did for the CI in this repo) or use PATH=/usr/lib/postgresql/11/bin stack test.

I guess I am not convinced the proposed solution is easer than those two options. I guess a third would be a wrapper script.

@tomjaguarpaw
Copy link
Collaborator Author

I guess I see a few reasons for this

  1. I don't like manually interacting with environment variables because they're global mutable state
  2. I don't like manually interacting with the shell or YAML files because they are untyped and generally not robust
  3. It would be nice if it simply worked out of the box for Debian users

One solution might be for the Config to have a new field indicating which initdb to use, one of

  1. Find it on PATH (status quo)
  2. Path to a specific initdb
  3. Search for it in well-known locations (including PATH, the Debian locations, maybe some others)

Then with/start could use option 3 but users could override it if necessary in withConfig/startConfig

@jfischoff
Copy link
Owner

I actually curious if you could do this now by augmenting the initDbConfig's environmentVariables to add the path there. And then maybe have a debianPathConfig :: IO Config types that could modified with other configs. Idk. Just spitballing.

I don't see prefixing the executable with the PATH as bad. If we include this configuration in the code we would have to maintain the paths as they change which would be annoying.

@jfischoff
Copy link
Owner

jfischoff commented Sep 11, 2020

Having it working out of the box sounds nice though. No idea how many people gave up on this because it is didn't work immediately.

@tomjaguarpaw
Copy link
Collaborator Author

I actually curious if you could do this now by augmenting the initDbConfig's environmentVariables to add the path there.

This is a bit weird.

  • environmentVariables is only mentioned in Config.hs.
  • The only non-trivial place it is read from is completeProcessConfig where it is used to determine completeProcessConfigEnvVars
  • completeProcessConfigEnvVars is only ultimately used in makeCommandLine (where it is only used to determine a hash, not to actually run anything)

So I'm not sure environmentVariables does what it is intended to do.

@tomjaguarpaw
Copy link
Collaborator Author

Having it working out of the box sounds nice though. No idea how many people gave up on this because it is didn't work immediately.

Hard to say, but there is something to be said for the most simple usage of the API being the one that works in the most use cases.

@tomjaguarpaw
Copy link
Collaborator Author

This is a bit weird.

Hmm, that's not right. It's also used in startProcess. But it doesn't work for my purpose because it sets the PATH of the created process, not the PATH in which the executable is looked up.

@jfischoff
Copy link
Owner

jfischoff commented Sep 11, 2020

So I'm not sure environmentVariables does what it is intended to do.

Completely possible that I am not remembering how to set it correctly.

Hmm, that's not right. It's also used in startProcess. But it doesn't work for my purpose because it sets the PATH of the created process, not the PATH in which the executable is looked up.

That's what I would think as well. However when there is no PATH there System.Process can't find the executable so it is used to find the executable and for the executables PATH ... or at least that is my guess. I'm not positive. I haven't extensively tested how the System.Process and the PATH works.

@jfischoff
Copy link
Owner

I think it is a good idea to add the paths by default. You have convinced me.

I would prefer if it was the default and there was a way to override it rather than extend the API with another function.

@tomjaguarpaw
Copy link
Collaborator Author

I think it is a good idea to add the paths by default. You have convinced me.

Great!

I would prefer if it was the default and there was a way to override it rather than extend the API with another function.

Yes, agreed.

@phadej
Copy link

phadej commented Aug 3, 2022

One alternative is to add fields to Config for program "names". By default they can be postgres, initdb, and createb, but users could specify absolute paths. That will by pass $PATH issues completely. And will be handy if someone would like to test with different postgres versions.

@ParetoOptimalDev
Copy link

Having it working out of the box sounds nice though. No idea how many people gave up on this because it is didn't work immediately.

I have a coworker who gave up for that reason and used their "whatever time" for some other tech debt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants