Fixes #38909 - Use syslog gem not in windows

[sbernhard]

No description provided.

[ekohl]

Could you open a Redmine issue for this?

I'd also love to have some CI on GHA that uses a Windows runner to test if it works, but lack the specific knowledge (and time). Do you have any idea how hard that would be?

[maximiliankolb]

@sbernhard Please create a Redmine issue & have a look at the failing tests. Please ping Ewoud after GHA are 🍏 for a final ACK.

edit: apologies; Redmine issue is open but GHA still fails. There must be another reason for that.

[sbernhard]

@sbernhard Please create a Redmine issue & have a look at the failing tests. Please ping Ewoud after GHA are 🍏 for a final ACK.

edit: apologies; Redmine issue is open but GHA still fails. There must be another reason for that.

everything done.

[ekohl]

@sbernhard the Redmine check is still failing: the commit must reference the issue.

[sbernhard]

Fixed it. Thanks @ekohl

[ekohl]

Looking at ruby/syslog@ffbfaa6 I wonder if it's really needed. I'd expect the gem (>= 0.2.0) to install on Windows and be a noop.

[ekohl]

By now this really evolved into just testing on Windows, which is still a good thing. Do you have any idea what is needed to actually make the tests work?

[ekohl]

C:/hostedtoolcache/windows/Ruby/3.3.10/x64/lib/ruby/3.3.0/bundled_gems.rb:69:in `require': cannot load such file -- libvirt (LoadError)

I think this (and the krb5) related tests should be skipped. The modules themselves (both dns_libvirt and dhcp_libvirt) both correctly guard behind load_classes so it should be possible.

The biggest risk I see is that we silently disable the tests without knowing it, but I'm sure we can find a way around that.

[sbernhard]

I think this (and the krb5) related tests should be skipped. The modules themselves (both dns_libvirt and dhcp_libvirt) both correctly guard behind load_classes so it should be possible.

I don't know how to skip the tests for these plugins. Can you please let me know more what you mean regarding load_classes??

[ekohl]

Can you please let me know more what you mean regarding load_classes??

In normal operation the Smart Proxy loads things in some order:

• The entry point is the "binary": smart-proxy or smart-proxy-win-service
• This loads smart_proxy_main

The main file is interesting because it loads all the plugins that are built in, but there's also the smart_proxy_for_testing which does NOT do that. I'll admit the exact details here are not entirely clear to me since it's been a while. I think for plugins from gems we rely on Bundler to include them, but load order is a bit vague.

However, those plugins are only the plugin definitions. Taking DNS as an example: dns/dns only loads dns/configuration_loader and dns/dns_plugin. Zooming in on Proxy::Dns::ConfigurationLoader we see it defines the load_classes method which loads the real implementation.

It's in the Proxy::PluginInitializer that actually really initializes the plugin. This is not a trivial class because it uses dependency injection, but I still think it makes a lot of sense. It's initialize_plugins that does a bunch of steps that eventually calls load_classes.

It allows for loading plugin definitions but having the plugin itself disabled via configuration. This is more efficient (less memory used) but can also help if the actual implementation is unavailable (like Kerberos or libvirt bindings that aren't installed.

But in the end this is all what runtime does. Testing is different: it loads many things unconditionally. Taking the failing libvirt test: the test library loads test/**/*_test.rb which in turn loads [test/dhcp_libvirt/libvirt_dhcp_network_test.rb]https://github.com/theforeman/smart-proxy/blob/develop/test/dhcp_libvirt/libvirt_dhcp_network_test.rb) which has this line:

smart-proxy/test/dhcp_libvirt/libvirt_dhcp_network_test.rb

Line 3 in 867f19e

require 'dhcp_libvirt/libvirt_dhcp_network'

At runtime this class is only loaded if it's really needed:

smart-proxy/modules/dhcp_libvirt/configuration_loader.rb

Lines 24 to 25 in 867f19e

	def load_classes
	require 'dhcp_libvirt/libvirt_dhcp_network'

That part is really loading the libvirt bindings:

smart-proxy/modules/dhcp_libvirt/libvirt_dhcp_network.rb

Lines 1 to 2 in 867f19e

	require 'libvirt'
	require 'libvirt_common/libvirt_network'

This is all a long intro into the loading mechanism. The real challenge is to modify the test suite so that it:

• Will test libvirt bindings if available
• Skip tests if bindings are unavailable
• Not accidentally skip the tests unconditionally

I don't have a good answer on that right now.

My had a thought about looking at which bundler groups were available. I couldn't find a good API for that, but didn't dig very deep. You can probably be blunt and use:

begin
  Bundler.require(:libvirt)
rescue LoadError # or whatever it throws
  LIBVIRT_BUNDLER_GROUP_AVAILABLE = false
else
  LIBVIRT_BUNDLER_GROUP_AVAILABLE = true
  require 'dhcp_libvirt/libvirt_dhcp_network'
end

Then based on https://test-unit.github.io/test-unit/en/Test/Unit/TestCaseOmissionSupport.html I'd try enhancing each test_ method with:

def test_dump_xml
  omit_unless(LIBVIRT_BUNDLER_GROUP_AVAILABLE) do
    a_xml = '<xml></xml>'
    @network.expects(:xml_desc).returns(a_xml)
    assert_equal a_xml, @subject.dump_xml
  end
end

And perhaps there's a nicer way. https://test-unit.github.io/test-unit/en/Test/Unit/TestCase.html may be helpful in seeing if that's an option.

Looking at that particular setup I see that also refers to ::Libvirt so that will also fail. So I'd also see if the following works:

class LibvirtDHCPNetworkTest < Test::Unit::TestCase
  def setup
    omit_unless(LIBVIRT_BUNDLER_GROUP_AVAILABLE)

    # rest of the existing code here
  end
end

Perhaps I'm also thinking too complex and perhaps Test::Unit already has something built in.