plugins: search for .a files if necessary
ClosedPublic

Authored by sheaf on Oct 23 2018, 8:15 AM.

Details

Summary

on windows, plugins are loaded via .a files,
but those paths were not being searched when loading plugins

Test Plan

./validate

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
sheaf created this revision.Oct 23 2018, 8:15 AM
Phyx requested changes to this revision.Oct 24 2018, 1:42 PM

Hi @sheaf Thanks for the patch.

Two things, can you re-enable the tests by undoing these changes https://phabricator.haskell.org/D5174#change-3onpbjV1AoZM so we can see if the patch works as expected.
Secondly I think this can be much simpler. I think we shouldn't care about DynWays at all. Just simple always check for static and dynamic libraries and just prefer dynamic over static.

This should be correct for all platforms and would results in simpler code here.

Thanks,
Tamar

This revision now requires changes to proceed.Oct 24 2018, 1:42 PM
sheaf updated this revision to Diff 18439.EditedOct 24 2018, 5:03 PM

Simpler plugin location search, as recommended by Phyx.

sheaf added a comment.Oct 24 2018, 5:07 PM

Thanks Phyx.

I think you were right, much better to directly search for library paths instead of going through "packageHsLibs" and messing with the DynFlags to get the output we want. I hope this hasn't messed with anything on other platforms! It's working on my end (on Windows) at least.

I hope I undid the right changes concerning the test-suite, let me know!

In D5253#144953, @sheaf wrote:

Thanks Phyx.

I think you were right, much better to directly search for library paths instead of going through "packageHsLibs" and messing with the DynFlags to get the output we want. I hope this hasn't messed with anything on other platforms! It's working on my end (on Windows) at least.

Hmm are you sure that plugins can't contain a profiled version? That seems a bit odd. @RyanGlScott might you know if they support profiling?

I hope I undid the right changes concerning the test-suite, let me know!

Yup you undid the right changes. The Windows build bot seems to be dead, I assume you verified this by running make test -C testsuite/tests/plugins/? The linux and macos builders should catch up soon.

sheaf added a comment.EditedOct 25 2018, 1:28 PM

Wasn't that what https://ghc.haskell.org/trac/ghc/ticket/15492 was about? I thought it showed that we should not be adding the _p suffix.
That might not be entirely true, but at least it shows that we don't want to add the _p suffix in all cases (in particular, not in the situation of that ticket). Doesn't that then mean that if we want to sometimes add _p, we would have to check for the WayDyn way to decide? I tried to avoid that as per your initial suggestion, but perhaps I misunderstood.

In D5253#145045, @Phyx wrote:

Hmm are you sure that plugins can't contain a profiled version? That seems a bit odd. @RyanGlScott might you know if they support profiling?

While I'm far from an expert on plugins, I don't see why they shouldn't support profiling. (This ticket, for instance, suggests that this is a supported use case.)

Phyx added a comment.Oct 25 2018, 1:45 PM

@RyanGlScott Yeah, looking at that ticket (same one @sheaf posted) seems to indicate that they are supported. I think you misunderstood the ticket @sheaf in that what it was attempting to solve was
that when DynWay isn't available, then ProfWay can't be either for plugins. But if Both DynWay and ProfWay are specified then it should work.

So their fix was to add DynWay to the flags if it's missing, but this is wrong since it assumes all playforms have DynWay. The correct fix would be to Check multiple variants just in a particular order.

First check with dflags unmodified and only then check by adding DynWays if it was missing from the original flags. This of course locks out using static libraries on a platform that uses DynWays but that's fine.

So you still need packageHsLibs.

sheaf updated this revision to Diff 18457.Oct 25 2018, 4:23 PM

Follow method recommended by Phyx to find plugin lib files.

sheaf added a comment.Oct 25 2018, 4:29 PM

Thanks for patiently walking me through this, I hope it's OK now. Locally it does correctly search for (and use) the profiled version of plugin libraries (e.g. libHS[...]_p.a).

Phyx added a comment.Oct 26 2018, 5:57 PM

Thanks @sheaf ! This looks good. Just one final small change and I'll accept it, you can combine the two list to simplify it,
move loc' to the same level as loc and combine them.

so have

locs = [ searchPath </> "lib" ++ libLoc <.> suffix
          | searchPath <- searchPaths
          , libLoc     <- packageHsLibs dflags pkg
          ]
dflags' = updateWays (addWay' WayDyn dflags)
locs'   = [ searchPath </> "lib" ++ libLoc <.> soExt platform
             | searchPath <- searchPaths
             , libLoc     <- packageHsLibs dflags' pkg
             , not useDyn
             ]
 paths = locs ++ locs`

This means we only have one error statement and makes the code a bit shorter and easier to follow. when useDyn the second list will just be empty then.

With this is should be good to go. Thanks again! Appreciate the patch!

sheaf updated this revision to Diff 18478.Oct 26 2018, 6:32 PM

Combine search paths to avoid duplication.

Phyx accepted this revision.Oct 27 2018, 2:10 AM

Thanks @sheaf!

This revision is now accepted and ready to land.Oct 27 2018, 2:10 AM
This revision was automatically updated to reflect the committed changes.