Skip to content

Conversation

@arielmol
Copy link

Tested with rocko.

@arielmol
Copy link
Author

So strange it did not build, my code does not even require to be compiled. It's basically a yocto recipe just for reference.

@peternewman
Copy link
Member

peternewman commented Apr 25, 2018

If you read why it's failing:
https://travis-ci.org/OpenLightingProject/ola/jobs/371260377#L7175

+./scripts/verify_trees.py ./ ola-0.10.6
Missing from tarball yocto/recipes-conectivity/ola/ola_git.bb
---------------------------------------
Either add the missing files to the appropriate Makefile.mk
or update IGNORE_PATTERNS in scripts/verify_trees.py
---------------------------------------

So you need to probably add a Makefile for that directory and add it to that.

Copy link
Member

@peternewman peternewman left a comment

Choose a reason for hiding this comment

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

A few comments on the build script.


PV = "0.10.6"

# Esta revision de git tiene un parche para protobuf, la tag de 0.10.6 no compila.
Copy link
Member

Choose a reason for hiding this comment

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

Could we have this comment in English please :)

Copy link
Author

Choose a reason for hiding this comment

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

Done

LICENSE = "GPLv2 & LGPLv2.1"
LIC_FILES_CHKSUM = "file://LICENCE;md5=7aa5f01584d845ad733abfa9f5cad2a1"

#DEPENDS_class-nativesdk = "bison libusb1 protobuf protobuf-native pkgconfig-native ossp-uuid ola-native "
Copy link
Member

Choose a reason for hiding this comment

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

Is this relevant still? Would this be for a native build?

Copy link
Author

@arielmol arielmol Apr 25, 2018

Choose a reason for hiding this comment

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

UPDATED: Read below

Copy link
Author

Choose a reason for hiding this comment

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

That line is optional for when you need an sdk

Copy link
Member

Choose a reason for hiding this comment

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

Should it be uncommented though then, there seems to be a mix of both referring to nativesdk throughout the file.


# target are the real config options
EXTRA_OECONF_append_class-target = " --with-ola-protoc-plugin=${STAGING_BINDIR_NATIVE}/ola_protoc_plugin --disable-python-libs\
--enable-http --enable-dummy --enable-gpio --enable-opendmx --enable-e131 \
Copy link
Member

Choose a reason for hiding this comment

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

You don't explicitly need to enable all the options. Although it will ensure configure fails if some of them aren't available for some reason.

Copy link
Author

Choose a reason for hiding this comment

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

In yocto anyone would be able to disable them, but disabling them will make ola build transparently but be useless as will have no plugins.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, if we need to explicitly enable all plugins in here we ought to add a note to ensure we add new ones here when they're added to https://github.com/OpenLightingProject/ola/blob/d34d4f13d954894e76bf024743e6519696813c29/configure.ac#L892-915

CXXFLAGS = "${CFLAGS}"

# The code is not Werror safe
do_configure_prepend() {
Copy link
Member

Choose a reason for hiding this comment

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

--disable-fatal-warnings will do this, without needing to be quite so hacky. Although we try our best to fix the issues, so ideally can you tell us what errors you're getting and we'll try and fix or bypass them before they become critical in future.

Copy link
Author

Choose a reason for hiding this comment

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

You mean as an argument for ./configure ?

Copy link
Member

Choose a reason for hiding this comment

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

Correct @desert .

But please either comment here, or pastebin the errors and we can try adn fix them properly.

@arielmol
Copy link
Author

So you need to probably add a Makefile for that directory and add it to that.

Oh, I know nothing about autotools.

@peternewman
Copy link
Member

peternewman commented May 17, 2018

So you need to probably add a Makefile for that directory and add it to that.

Oh, I know nothing about autotools.

That's easy, see the Debian one here:
https://github.com/OpenLightingProject/ola/blob/master/debian/Makefile.mk

And then the new Makefile.mk needs including in the master Makefile.am:
https://github.com/OpenLightingProject/ola/blob/master/Makefile.am#L153

@peternewman peternewman added this to the 0.11.0 milestone Jul 28, 2018
Copy link
Member

@peternewman peternewman left a comment

Choose a reason for hiding this comment

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

Some updated comments/re-review.

Comment on lines +12 to +16
PV = "0.10.6"

# 0.10.6 tag won't build, this rev has protobuf patches
SRCREV = "00dc86a48ec4c528cec90166435b440f283a9c86"
SRC_URI = "git://github.com/OpenLightingProject/ola.git;protocol=https"
Copy link
Member

Choose a reason for hiding this comment

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

We should be able to build 0.10.9 cleanly now, is this the right sort of syntax?

Suggested change
PV = "0.10.6"
# 0.10.6 tag won't build, this rev has protobuf patches
SRCREV = "00dc86a48ec4c528cec90166435b440f283a9c86"
SRC_URI = "git://github.com/OpenLightingProject/ola.git;protocol=https"
PV = "0.10.9"
SRC_URI = "git://github.com/OpenLightingProject/ola.git;protocol=https"

LICENSE = "GPLv2 & LGPLv2.1"
LIC_FILES_CHKSUM = "file://LICENCE;md5=7aa5f01584d845ad733abfa9f5cad2a1"

#DEPENDS_class-nativesdk = "bison libusb1 protobuf protobuf-native pkgconfig-native ossp-uuid ola-native "
Copy link
Member

Choose a reason for hiding this comment

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

Should it be uncommented though then, there seems to be a mix of both referring to nativesdk throughout the file.


inherit autotools-brokensep relative_symlinks
# autotools-brokensep
# forces in-source building (ola has bugs for out-of-tree builds).
Copy link
Member

Choose a reason for hiding this comment

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

I think/hope we've fixed this too now...

# inherit relative_symlinks within the recipe to turn those absolute symlinks into relative
# https://www.yoctoproject.org/docs/2.4/ref-manual/ref-manual.html#migration-2.3-absolute-symlinks

#EXTRA_OECONF = " --disable-unittests --disable-rdm-tests --disable-python-libs --disable-java-libs --disable-all-plugins"
Copy link
Member

Choose a reason for hiding this comment

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

Is this line redundant/part of testing?


# target are the real config options
EXTRA_OECONF_append_class-target = " --with-ola-protoc-plugin=${STAGING_BINDIR_NATIVE}/ola_protoc_plugin --disable-python-libs\
--enable-http --enable-dummy --enable-gpio --enable-opendmx --enable-e131 \
Copy link
Member

Choose a reason for hiding this comment

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

Okay, if we need to explicitly enable all plugins in here we ought to add a note to ensure we add new ones here when they're added to https://github.com/OpenLightingProject/ola/blob/d34d4f13d954894e76bf024743e6519696813c29/configure.ac#L892-915

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