add HOST_PATH support by matthewbauer · Pull Request #1 · obsidiansystems/which

matthewbauer · 2019-09-11T22:48:04Z

HOST_PATH is set in Nixpkgs during cross-compilation to detect binaries available at runtime. It makes sure you get the right architecture in your cross-compiled binaries. When HOST_PATH is not set, fall back to PATH.

This does what we want

findExecutablesInDirectories was only added in 1.2.4.0.

ali-abrar

Please also update the changelog and address CI failures. Thanks!

ali-abrar · 2019-09-12T01:15:49Z

 which :: FilePath -> IO (Maybe FilePath)
-which f = fmap (fmap (T.unpack . Sh.toTextIgnore)) $ Sh.shelly $ Sh.which $ Sh.fromText $ T.pack f
+which f = do
+  path <- runMaybeT $ MaybeT (lookupEnv "HOST_PATH") <|> MaybeT (lookupEnv "PATH")


How is the behavior different now compared to when we used which from Shelly?

The only difference is that HOST_PATH is now used when it is available.

ali-abrar · 2019-09-12T01:16:53Z

-      | otherwise -> compileError $ "Path to executable " <> show f <> " was found in " <> show f' <> " which is not in /nix/store. Be sure to add the relevant package to 'backendTools' in default.nix."
+      | otherwise -> compileError $
+        "Path to executable " <> show f <> " was found in " <> show f'
+        <> " which is not in /nix/store. Be sure to add the relevant package to 'backendTools' in default.nix."


This comment assumes that this package is being used in the context of an obelisk project, it seems. The reference to "backendTools in default.nix" should be changed.

Oops, I removed this for my rhyolite PR https://github.com/obsidiansystems/rhyolite/pull/100/files#diff-8e11fb146d567575b09cdf4138230cebR24, but then copied my project's which to a separate package and accidentally re-introduced this.

ali-abrar · 2019-09-12T01:17:08Z

+  appendFile (bin3 </> "hello") "hello"
+  makeExecutable $ bin3 </> "hello"
+  appendFile (bin3 </> "hello2") "hello2"
+  -- makeExecutable $ bin3 </> "hello2"


Why is this commented out?

This was just to make sure that non-executable files don't end up in which results.

ali-abrar · 2019-09-12T01:17:40Z

+
+makeExecutable :: FilePath -> IO ()
+makeExecutable f = setPermissions f . setOwnerExecutable True =<< getPermissions f
+


Is there a test here that fails with the old Shelly implementation but passes now?

Probably not, this was just to make sure the new implementation of which is correct.

ryantrinkle · 2019-09-12T01:39:25Z

What is HOST_PATH?

…

On Wed, Sep 11, 2019, 21:18 Ali Abrar ***@***.***> wrote: ***@***.**** requested changes on this pull request. Please also update the changelog. ------------------------------ In src/System/Which.hs <#1 (comment)>: > which :: FilePath -> IO (Maybe FilePath) -which f = fmap (fmap (T.unpack . Sh.toTextIgnore)) $ Sh.shelly $ Sh.which $ Sh.fromText $ T.pack f +which f = do + path <- runMaybeT $ MaybeT (lookupEnv "HOST_PATH") <|> MaybeT (lookupEnv "PATH") How is the behavior different now compared to when we used which from Shelly? ------------------------------ In src/System/Which.hs <#1 (comment)>: > staticWhich :: FilePath -> Q Exp staticWhich f = do mf' <- runIO $ which f case mf' of Nothing -> compileError $ "Could not find executable for " <> show f Just f' | "/nix/store/" `isPrefixOf` f' -> [| f' |] - | otherwise -> compileError $ "Path to executable " <> show f <> " was found in " <> show f' <> " which is not in /nix/store. Be sure to add the relevant package to 'backendTools' in default.nix." + | otherwise -> compileError $ + "Path to executable " <> show f <> " was found in " <> show f' + <> " which is not in /nix/store. Be sure to add the relevant package to 'backendTools' in default.nix." This comment assumes that this package is being used in the context of an obelisk project, it seems. The reference to "backendTools in default.nix" should be changed. ------------------------------ In test/Spec.hs <#1 (comment)>: > +setup :: IO (FilePath, FilePath, FilePath) +setup = do + bin <- mkdtemp "bin" + bin2 <- mkdtemp "bin2" + bin3 <- mkdtemp "bin3" + + appendFile (bin </> "hello") "hello" + makeExecutable $ bin </> "hello" + appendFile (bin2 </> "hello") "hello2" + makeExecutable $ bin2 </> "hello" + appendFile (bin2 </> "hello2") "hello2" + makeExecutable $ bin2 </> "hello2" + appendFile (bin3 </> "hello") "hello" + makeExecutable $ bin3 </> "hello" + appendFile (bin3 </> "hello2") "hello2" + -- makeExecutable $ bin3 </> "hello2" Why is this commented out? ------------------------------ In test/Spec.hs <#1 (comment)>: > +import System.Directory +import System.Environment +import System.Exit +import System.FilePath +import System.Posix.Temp +import System.Which + +shouldEqual :: Eq a => Show a => a -> a -> IO () +shouldEqual a b | a /= b = do + putStrLn $ show a <> " does not equal " <> show b + exitFailure +shouldEqual a b = putStrLn $ show a <> " == " <> show b + +makeExecutable :: FilePath -> IO () +makeExecutable f = setPermissions f . setOwnerExecutable True =<< getPermissions f + Is there a test here that fails with the old Shelly implementation but passes now? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1?email_source=notifications&email_token=AAI2KYDOKC3WZUABFGPGFX3QJGKEZA5CNFSM4IVZURI2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCEOZCHA#pullrequestreview-287150364>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAI2KYFX73LAUTGGTGSIH5DQJGKEZANCNFSM4IVZURIQ> .

Ericson2314 · 2019-09-12T03:28:26Z

@ryantrinkle HOST_PATH is a Nix thing Matt and I added to distinguish between build-time executables and run-time executables, so when you cross compile you get a binary you can actually use.

ryantrinkle · 2019-09-12T15:02:37Z

This should be done in a package called which-nix rather than which.

matthewbauer · 2019-09-12T17:21:09Z

This should be done in a package called which-nix rather than which.

staticWhich already requires that which results be in /nix/store, so this is already nix-specific. In addition, there's no reason that HOST_PATH needs to be just for Nix. Any time we cross compile with "staticWhich", we will end up getting binaries from the build machine that are the wrong architecture for the target machine. Setting HOST_PATH provides a way to get around that behavior. Otherwise everything using static-which can't cross compile correctly. If there's a better name than HOST_PATH available we can switch to that.

3noch · 2020-06-05T14:46:39Z

I think we just need a version of which that lets you pick which env vars to search instead of hardcoding "arbitrary" choices like this here. Also splitting out any nix-specific logic into a new package is the right move. It can wrap this package and add nix-specific functionality/checks. That's the direction we should go I think.

matthewbauer · 2020-07-30T23:27:42Z

I think we just need a version of which that lets you pick which env vars to search instead of hardcoding "arbitrary" choices like this here. Also splitting out any nix-specific logic into a new package is the right move. It can wrap this package and add nix-specific functionality/checks. That's the direction we should go I think.

So, we could make something like whichWithPath that you could use to specify your own path variable. But then, I'm afraid that everyone would just do whichWithPath "PATH" and cross-compilation would be broken. When using which, all we need to know is whether you will be running the executable at runtime or buildtime. Right now, almost all usages of which are for runtime. I guess if people really object to reading HOST_PATH directly in which, we could add whichWithPaths and make everyone do whichWithPaths ["HOST_PATH" "PATH"] when they want cross-compilation to work.

matthewbauer added 10 commits September 11, 2019 17:43

Format long lines

9ee3342

Specify exports in System.Which

c9c16e2

Use HOST_PATH where available

bb693e1

HOST_PATH is set in Nixpkgs during cross-compilation to detect binaries available at runtime. It makes sure you get the right architecture in your cross-compiled binaries. When HOST_PATH is not set, fall back to PATH.

Add gitignore

b392f92

Use findExecutablesInDirectories from directory

2c77380

This does what we want

Add test suite

dd815ee

Bump version to 0.2.0.0

19e1c47

Cleanup

0ebf7c0

Tighten directory bounds

16cdd63

findExecutablesInDirectories was only added in 1.2.4.0.

Cleanup test suite

88a23fb

ali-abrar requested changes Sep 12, 2019

View reviewed changes

matthewbauer added 3 commits September 12, 2019 10:31

Fix missing mappend in pre-8.4 ghc

a437030

Update template haskell error

f3e516b

Add comment on bin3/hello2

e22ae84

matthewbauer mentioned this pull request Sep 12, 2019

Allow which results outside of /nix/store #3

Merged

matthewbauer added 2 commits July 30, 2020 18:29

Merge remote-tracking branch 'origin/develop' into host-which

0b41a95

Remove comment on Spec.hs

e230421


		makeExecutable :: FilePath -> IO ()
		makeExecutable f = setPermissions f . setOwnerExecutable True =<< getPermissions f

Conversation

matthewbauer commented Sep 11, 2019

Uh oh!

ali-abrar left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryantrinkle commented Sep 12, 2019 via email

Uh oh!

Ericson2314 commented Sep 12, 2019

Uh oh!

ryantrinkle commented Sep 12, 2019

Uh oh!

matthewbauer commented Sep 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

3noch commented Jun 5, 2020

Uh oh!

matthewbauer commented Jul 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ali-abrar left a comment •

edited

Loading

matthewbauer commented Sep 12, 2019 •

edited

Loading

matthewbauer commented Jul 30, 2020 •

edited

Loading