Skip to content

migrate all plugin descriptions to Markdown - #1185

Merged
peternewman merged 13 commits into
OpenLightingProject:masterfrom
FloEdelmann:plugins-md2
Jan 8, 2017
Merged

migrate all plugin descriptions to Markdown#1185
peternewman merged 13 commits into
OpenLightingProject:masterfrom
FloEdelmann:plugins-md2

Conversation

@FloEdelmann

Copy link
Copy Markdown
Member

see #1142

@peternewman peternewman self-assigned this Jan 7, 2017
@peternewman peternewman added this to the 0.11.0 milestone Jan 7, 2017

@peternewman peternewman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @FloEdelmann . I've fixed the basic lint issues where the include files are in the wrong order, but the issues with OPC and USB Serial will need some more work.

Specifically this line here:
https://github.com/OpenLightingProject/ola/blob/master/plugins/convert_README_to_header.sh#L36

It's complaining about this:
./plugins/openpixelcontrol/OPCPluginDescription.h:22: #ifndef header guard has wrong style, please use: PLUGINS_OPENPIXELCONTROL_OPCPLUGINDESCRIPTION_H_ [build/header_guard] [5]

Because your current guard is PLUGINS_OPENPIXELCONTROL_OPENPIXELCONTROLPLUGINDESCRIPTION_H_ . I think you just need to take outfilename, strip the .cpp off the end and you should be okay.

All the compilations are failing with:
sh ./plugins/convert_README_to_header.sh ./plugins/artnet ./plugins/artnet/ArtNetPluginDescription.h
make[1]: *** No rule to make target plugins/dmx4linux/Dmx4LinuxPluginDescription.h', needed by distdir'. Stop.

I can't immediately see why the ArtNet one is working, but not the Dmx4Linux one.

@peternewman

peternewman commented Jan 7, 2017

Copy link
Copy Markdown
Member

Looking further, the ArtNet plugin has no dependencies, whereas Dmx4Linux does, because it doesn't have the dependencies, and hence it's not building the Description.h, is I think why it's failing.

I think you may be able to solve this by pulling this bit of code outside the if USE_DMX4LINUX bit:

# Plugin description is generated from README.md
built_sources += plugins/dmx4linux/Dmx4LinuxPluginDescription.h
plugins/dmx4linux/Dmx4LinuxPluginDescription.h: plugins/dmx4linux/README.md plugins/dmx4linux/Makefile.mk plugins/convert_README_to_header.sh
	sh $(top_srcdir)/plugins/convert_README_to_header.sh $(top_srcdir)/plugins/dmx4linux $(top_builddir)/plugins/dmx4linux/Dmx4LinuxPluginDescription.h

In which case you'll need to do it for all the others too.

@FloEdelmann

Copy link
Copy Markdown
Member Author

On Mac OS, the WidgetDetectorThreadTest.cpp fails at line 367:

Test name: WidgetDetectorThreadTest::testUsbProWidget
equality assertion failed
- Expected: 2
- Actual  : 0

But I don't know why because I didn't change anything relevant to this. And it also doesn't fail on Linux.

@peternewman peternewman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Firstly you can ignore the WidgetDetectorThreadTest::testUsbProWidget failure. It's because I updated the version of xcode we're using on Travis and that bug appeared, I think it's a race condition, and I think I've fixed it now, but we can ignore it and merge regardless anyway.

I've realised I think I've given you some duff info, having read up more, and looked at what we've done elsewhere, we should keep the code within the USE_DMX4LINUX bit, but change it to not build during make dist, only when it's actually compiling.

See e.g. plugins/artnet/messages/ArtNetConfigMessages.pb.cc in https://github.com/OpenLightingProject/ola/blob/master/plugins/artnet/messages/Makefile.mk

So I think you need to put it back in USE_DMX4LINUX and add:

nodist_plugins_dmx4linux_liboladmx4linux_la_SOURCES = \
    plugins/dmx4linux/Dmx4LinuxPluginDescription.h

Then remove that file from plugins_dmx4linux_liboladmx4linux_la_SOURCES, so it's not built during dist, but is used the rest of the time.

You also need to do this for all plugins (not just SPI, uartdmx and DMX4Linux), as this means it will continue to work if the machine running make dist is also missing other libraries (or potentially has them disabled while running configure), note how the Linux machine didn't mind about uartdmx, only the Mac did, as the latter never builds that plugin as it's not supported.

@FloEdelmann

Copy link
Copy Markdown
Member Author

Seems to work now :)

@peternewman
peternewman merged commit 5f490bd into OpenLightingProject:master Jan 8, 2017
@peternewman

Copy link
Copy Markdown
Member

Thanks for all your hard work @FloEdelmann . Do you fancy attempting the other couple of small tasks in the project?
https://github.com/OpenLightingProject/ola/projects/1

@FloEdelmann
FloEdelmann deleted the plugins-md2 branch January 8, 2017 21:45
@FloEdelmann

FloEdelmann commented Jan 8, 2017

Copy link
Copy Markdown
Member Author

I can try but I'll focus on my exams in university the next weeks. But after that, vacation is the perfect time for OLA ;)

@peternewman

Copy link
Copy Markdown
Member

Okay, well thanks for the work so far @FloEdelmann . I may have a look at the other bits when I get a chance.

peternewman added a commit to peternewman/ola that referenced this pull request Feb 24, 2017
peternewman added a commit that referenced this pull request Feb 25, 2017
Fix an error that snuck in with #1185
@peternewman peternewman mentioned this pull request Jul 12, 2026
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

Successfully merging this pull request may close these issues.

2 participants