Project

General

Profile

Feature #17900

Allow configuration of MessageLoggerScribe's plugin factory search paths

Added by Ben Morgan over 3 years ago. Updated about 1 year ago.

Status:
Assigned
Priority:
Normal
Assignee:
-
Target version:
-
Start date:
10/13/2017
Due date:
% Done:

0%

Estimated time:
2.00 h
Duration:

Description

With #11678 available for a while, I've had some time to look at upstream usage of LibraryManager and PluginFactory. The first main ones look to be here, in source:messagefacility/MessageLogger/MessageLoggerScribe.h#L63. On macOS (Sierra, Xcode 9, SIP enabled) the tests of MF plugins fail as expected. Hacking the two plugin factory definitions to:

cet::BasicPluginFactory pluginFactory_{cet::search_path{"MF_PLUGIN_PATH"},"mfPlugin"};
cet::BasicPluginFactory pluginFactory_{cet::search_path{"MF_PLUGIN_PATH"},"mfStatsPlugin"};

and setting MF_PLUGIN_PATH to the build time location of the plugins when running the tests makes all tests pass, but of course that's just a proof of principle.

I'm happy to provide patches to support making the MF plugin search paths configurable, but obviously any changes need to be considered for compatibility/configuration changes, so wanted advance from the developers before going ahead with things.

I guess the initial question would be where such a configuration option should come in - as an environment variable, or via FHiCL, or both - and how to modify the interfaces needed in the correct way?

0001-Use-MF_PLUGIN_PATH-env-var-for-plugins-is-present.patch (3.37 KB) 0001-Use-MF_PLUGIN_PATH-env-var-for-plugins-is-present.patch Ben Morgan, 11/20/2017 09:38 AM

History

#1 Updated by Marc Paterno over 3 years ago

  • Status changed from New to Feedback

We would be happy to receive patches that implement the support for a SIP-enabled system (although we are not directly supporting that ourselves at this time).

We agree with your proposed use of an environment variable (different from DYLD_LIBRARY_PATH) to use as the search path for the MF plugins. Perhaps MF_PLUGIN_PATH could be set in the UPS table files, as is DYLD_LIBRARY_PATH currently.

We are concerned with difficulty in maintenance, and so wish to avoid conditional compilation. And, as you noted, any changes must not break existing use (on a SIP-disabled machine, and on Linux).

#2 Updated by Ben Morgan about 3 years ago

Marc Paterno wrote:

We would be happy to receive patches that implement the support for a SIP-enabled system (although we are not directly supporting that ourselves at this time).

We agree with your proposed use of an environment variable (different from DYLD_LIBRARY_PATH) to use as the search path for the MF plugins. Perhaps MF_PLUGIN_PATH could be set in the UPS table files, as is DYLD_LIBRARY_PATH currently.

We are concerned with difficulty in maintenance, and so wish to avoid conditional compilation. And, as you noted, any changes must not break existing use (on a SIP-disabled machine, and on Linux).

Thanks Marc, I don't think conditional compilation will be needed, so I'll look at implementing a first iteration to

  1. Use MF_PLUGIN_PATH if it is set in the environment
  2. Fallback to (DY)LD_LIBRARY_PATH otherwise

Should I consider the case of both being set (combine, prefer one)?

#3 Updated by Kyle Knoepfel about 3 years ago

The behavior you describe seems sensible to us. If both are set, prefer MF_PLUGIN_PATH.

#4 Updated by Ben Morgan about 3 years ago

I've attached a WIP patch for review (based on tag 2_01_01- also viewable online at : https://github.com/drbenmorgan/fnal-messagefacility/commit/1b8ea88abfa58291d7f170ff9228daec2a636ced

The basic behaviour is implemented as discussed earlier

  1. Construct the two BasicPluginFactory instances with a cet::search_path that uses
    1. Environment variable MF_PLUGIN_PATH if it is set in the environment
    2. OS-specific dynamic loader path if not

Other than basic structure/style issues, I wasn't sure what to do on checking the exception category here on L67 It should be "getenv" but not sure on CET policy for checking this.

It's also not clang-formatted yet, so I can rebase onto the latest tag if needed.

Tests now pass on both macOS (SIP enabled, using non-UPS build) if MF_PLUGIN_PATH is set manually, and on Linux (using cetbuildtools) using LD_LIBRARY_PATH set by UPS

I wasn't sure how you'd prefer to test these cases, so have left source:messagefacility/test/CMakeLists.txt as-is. To test use of MF_PLUGIN_PATH would be a simple case of adding the line

# - MF_PLUGIN_PATH
cet_test_env("MF_PLUGIN_PATH=$<TARGET_FILE_DIR:MF_Utilities>")

to that file ((DY)LD_LIBRARY_PATH) can be set in a similar way if required).

#5 Updated by Christopher Green about 3 years ago

  • Status changed from Feedback to Assigned
  • Assignee set to Paul Russo
  • Estimated time set to 2.00 h

Thanks for this, Ben. We will review the patch and apply as appropriate.

#6 Updated by Kyle Knoepfel about 1 year ago

  • Assignee deleted (Paul Russo)

Also available in: Atom PDF