Skip to content

ARROW-5631: [C++] Fix FindBoost targets with cmake3.2 - #4605

Closed
fsaintjacques wants to merge 8 commits into
apache:masterfrom
fsaintjacques:ARROW-5631-cmake32-boost
Closed

ARROW-5631: [C++] Fix FindBoost targets with cmake3.2#4605
fsaintjacques wants to merge 8 commits into
apache:masterfrom
fsaintjacques:ARROW-5631-cmake32-boost

Conversation

@fsaintjacques

Copy link
Copy Markdown
Contributor

cmake's FindBoost pre 3.5 did not export IMPORTED targets. This also fixes the newly introduced trusty docker image.

@fsaintjacquesfsaintjacquesJun 18, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I'm not sure why we need to stash BUILD_SHARED_LIBS, if someones wants to clarify, I'll gladly add a comment to the PR (@xhochy, @wesm).

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.

This is needed to work with BoostConfig.cmake provided by Boost (not CMake).
The Boost package provided by MSYS2 installed BoostConfig.cmake at the time.
But the current Boost package doesn't install BoostConfig.cmake: https://github.com/msys2/MINGW-packages/pull/5326/files

See also: msys2/MINGW-packages#5233

There is no platform that needs this. So we can remove this for now but we may need this again in the future.

@fsaintjacques

Copy link
Copy Markdown
ContributorAuthor

A note that this patch does not fully fix the cmake3.2 issue, see https://jira.apache.org/jira/browse/ARROW-5631?filter=-1

But at least it gets a cmake build directory populated.

@wesm

wesm commented Jun 18, 2019

Copy link
Copy Markdown
Member

Looks like @kou introduced that logic here

f014d76

I'm not sure what it's for as we don't use this flag other places in our build system. Some projects use the flag to globally toggle between building SHARED/STATIC when using add_library but we are explicit about this everywhere.

@fsaintjacques

Copy link
Copy Markdown
ContributorAuthor

@kou afaik, find_package shouldn't be affected by BUILD_SHARED_LIBS?

@wesm

wesm commented Jun 18, 2019

Copy link
Copy Markdown
Member

I think it's fine to keep the stashing; it seems like it was more likely future proofing

@fsaintjacques

Copy link
Copy Markdown
ContributorAuthor

Don't merge for now, it doesn't fix the docker cmake32 image.

@fsaintjacquesfsaintjacques changed the title ARROW-5631: [C++] Fix FindBoost targets with cmake3.2[WIP] ARROW-5631: [C++] Fix FindBoost targets with cmake3.2Jun 18, 2019
@fsaintjacques
fsaintjacquesforce-pushed the ARROW-5631-cmake32-boost branch from c4d7d34 to 033a944CompareJune 19, 2019 15:12
@fsaintjacquesfsaintjacques changed the title [WIP] ARROW-5631: [C++] Fix FindBoost targets with cmake3.2ARROW-5631: [C++] Fix FindBoost targets with cmake3.2Jun 19, 2019
@fsaintjacques
fsaintjacquesforce-pushed the ARROW-5631-cmake32-boost branch from 033a944 to 8a89596CompareJune 20, 2019 19:37
@fsaintjacques

fsaintjacques commented Jun 20, 2019

Copy link
Copy Markdown
ContributorAuthor

rebased with master, ready to land (once it passes tests). This passes locally, minus the linking error fixed by #4635.

@kou

kou commented Jun 20, 2019

Copy link
Copy Markdown
Member

It seems that the current changes don't fix anything.
Should we really fix this problem or update title and description of this pull request?

@fsaintjacques

Copy link
Copy Markdown
ContributorAuthor

@kou previously cmake didn't even succeed at generating a build folder. The ctest failures are fixed in the PR linked in previous comment (#4635).

@codecov-io

Copy link
Copy Markdown

Codecov Report

Merging #4605 into master will increase coverage by 0.41%.
The diff coverage is n/a.

Impacted file tree graph

@@ Coverage Diff @@## master #4605 +/- ##
==========================================
+ Coverage 88.49% 88.91% +0.41% 
==========================================
Files 890 710 -180 Lines 110406 96682 -13724 Branches 1418 0 -1418 ==========================================
- Hits 97703 85960 -11743 + Misses 12422 10722 -1700 + Partials 281 0 -281
Impacted FilesCoverage Δ
go/arrow/ipc/writer.go
go/arrow/math/uint64_amd64.go
go/arrow/memory/memory_avx2_amd64.go
go/arrow/ipc/file_reader.go
js/src/builder/index.ts
js/src/enum.ts
go/arrow/array/builder.go
js/src/util/fn.ts
js/src/Arrow.node.ts
js/src/schema.ts
... and 170 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a0e1fbb...891407e. Read the comment docs.

@fsaintjacques

Copy link
Copy Markdown
ContributorAuthor

@ursabot crossbox --help

@ursabot

Copy link
Copy Markdown
No such command "crossbox".

@fsaintjacques

Copy link
Copy Markdown
ContributorAuthor

@ursabot crossbow --help

@ursabot

Copy link
Copy Markdown
Usage: @ursabot crossbow [OPTIONS] COMMAND [ARGS]...
Trigger crossbow builds for this pull request
Options:
--help Show this message and exit.
Commands:
package Submit crossbow packaging tasks.
test Submit crossbow testing tasks.

@fsaintjacques

Copy link
Copy Markdown
ContributorAuthor

@ursabot crossbow test

@ursabot

Copy link
Copy Markdown

AMD64 Conda Crossbow (#23233) builder failed.

Revision: f097a4b4ea5ae527a6cf8bfc7a927e5dbca5616b

Crossbow: 'python crossbow.py ...' step's stderr:

Traceback (most recent call last):
File "crossbow.py", line 924, in <module>
crossbow(obj={}, auto_envvar_prefix='CROSSBOW')
File "/opt/conda/lib/python3.7/site-packages/click/core.py", line 764, in __call__
return self.main(*args, **kwargs)
File "/opt/conda/lib/python3.7/site-packages/click/core.py", line 717, in main
rv = self.invoke(ctx)
File "/opt/conda/lib/python3.7/site-packages/click/core.py", line 1137, in invoke
return _process_result(sub_ctx.command.invoke(sub_ctx))
File "/opt/conda/lib/python3.7/site-packages/click/core.py", line 956, in invoke
return ctx.invoke(self.callback, **ctx.params)
File "/opt/conda/lib/python3.7/site-packages/click/core.py", line 555, in invoke
return callback(*args, **kwargs)
File "/opt/conda/lib/python3.7/site-packages/click/decorators.py", line 17, in new_func
return f(get_current_context(), *args, **kwargs)
File "crossbow.py", line 751, in submit
job = Job(target=target, tasks=tasks)
File "crossbow.py", line 574, in __init__
raise ValueError('no tasks were provided for the job')
ValueError: no tasks were provided for the job

@fsaintjacques

Copy link
Copy Markdown
ContributorAuthor

@ursabot crossbow test -g docker

@ursabot

Copy link
Copy Markdown

AMD64 Conda Crossbow (#23234) builder has been succeeded.

Revision: f097a4b4ea5ae527a6cf8bfc7a927e5dbca5616b

Submitted crossbow builds: ursa-labs/crossbow @ ursabot-41

TaskStatus
docker-javaCircleCI Status
docker-jsCircleCI Status
docker-python-3.6-alpineCircleCI Status
docker-python-3.6-nopandasCircleCI Status
docker-rustCircleCI Status
docker-python-3.7CircleCI Status
docker-cpp-alpineCircleCI Status
docker-goCircleCI Status
docker-docsCircleCI Status
docker-python-2.7-alpineCircleCI Status
docker-cpp-cmake32CircleCI Status
docker-python-2.7-nopandasCircleCI Status
docker-clang-formatCircleCI Status
docker-rCircleCI Status
docker-lintCircleCI Status
docker-dask-integrationCircleCI Status
docker-python-2.7CircleCI Status
docker-hdfs-integrationCircleCI Status
docker-turbodbc-integrationCircleCI Status
docker-python-3.6CircleCI Status
docker-pandas-masterCircleCI Status
docker-iwyuCircleCI Status
docker-cppCircleCI Status
docker-spark-integrationCircleCI Status
docker-c_glibCircleCI Status

@fsaintjacques
fsaintjacquesforce-pushed the ARROW-5631-cmake32-boost branch from f097a4b to 0e11383CompareJune 21, 2019 14:55
@fsaintjacques

Copy link
Copy Markdown
ContributorAuthor

@ursabot crossbow test docker docker-cpp docker-cpp-cmake32 docker-rust

@ursabot

Copy link
Copy Markdown

AMD64 Conda Crossbow (#23279) builder failed.

Revision: 0e11383c1589ac73b2a8abb2c158038a1477de40

Crossbow: 'python crossbow.py ...' step's stderr:

Error: Invalid task(s) {'docker'}. Must be one of {'docker-clang-format', 'docker-rust', 'docker-dask-integration', 'docker-python-3.6-alpine', 'docker-lint', 'docker-python-2.7-alpine', 'docker-python-3.6-nopandas', 'docker-python-2.7-nopandas', 'docker-c_glib', 'docker-python-2.7', 'docker-python-3.6', 'docker-pandas-master', 'docker-js', 'docker-java', 'docker-turbodbc-integration', 'docker-hdfs-integration', 'docker-cpp-alpine', 'docker-iwyu', 'docker-python-3.7', 'docker-cpp', 'docker-cpp-cmake32', 'docker-go', 'docker-spark-integration', 'docker-r', 'docker-docs'}

@fsaintjacques

Copy link
Copy Markdown
ContributorAuthor

@ursabot crossbow test docker-cpp docker-cpp-cmake32 docker-rust

@ursabot

Copy link
Copy Markdown

AMD64 Conda Crossbow (#23280) builder has been succeeded.

Revision: 0e11383c1589ac73b2a8abb2c158038a1477de40

Submitted crossbow builds: ursa-labs/crossbow @ ursabot-42

TaskStatus
docker-cppCircleCI Status
docker-cpp-cmake32CircleCI Status
docker-rustCircleCI Status

@fsaintjacques

Copy link
Copy Markdown
ContributorAuthor

@ursabot crossbow test docker-c_glib

@ursabot

Copy link
Copy Markdown

AMD64 Conda Crossbow (#23290) builder has been succeeded.

Revision: 0e11383c1589ac73b2a8abb2c158038a1477de40

Submitted crossbow builds: ursa-labs/crossbow @ ursabot-43

TaskStatus
docker-c_glibCircleCI Status

@fsaintjacques

Copy link
Copy Markdown
ContributorAuthor

@kszucs do you know why the badge is at failed? The last one are waiting to be run.

@fsaintjacques

Copy link
Copy Markdown
ContributorAuthor

@kou could you check locally with this patch why c_glib docker image fails? I've exausted my debugging of meson. I think that you need to namespace gandiva's enums.c.

gandiva-glib/meson.build:50:0: ERROR: Tried to create target "enums.c", but a target of that name already exists.

I'll let you decide if renaming breaks anything.

@fsaintjacques
fsaintjacquesforce-pushed the ARROW-5631-cmake32-boost branch from 0b1a568 to dbf6fe7CompareJune 21, 2019 16:13
@fsaintjacques

Copy link
Copy Markdown
ContributorAuthor

@ursabot crossbow test docker-c_glib

@ursabot

Copy link
Copy Markdown

AMD64 Conda Crossbow (#23312) builder has been succeeded.

Revision: dbf6fe7d9d813e1179d697c7eeab0b31900257b1

Submitted crossbow builds: ursa-labs/crossbow @ ursabot-44

TaskStatus
docker-c_glibCircleCI Status

@fsaintjacques

Copy link
Copy Markdown
ContributorAuthor

@ursabot crossbow test docker-r

@ursabot

Copy link
Copy Markdown

AMD64 Conda Crossbow (#23410) builder has been succeeded.

Revision: 46d5da0b3b08be423333e17e7206a824f437162e

Submitted crossbow builds: ursa-labs/crossbow @ ursabot-45

TaskStatus
docker-rCircleCI Status

@fsaintjacques

Copy link
Copy Markdown
ContributorAuthor

@ursabot crossbow test -g docker

@ursabot

Copy link
Copy Markdown

AMD64 Conda Crossbow (#23414) builder has been succeeded.

Revision: 46d5da0b3b08be423333e17e7206a824f437162e

Submitted crossbow builds: ursa-labs/crossbow @ ursabot-46

TaskStatus
docker-iwyuCircleCI Status
docker-dask-integrationCircleCI Status
docker-hdfs-integrationCircleCI Status
docker-python-3.6CircleCI Status
docker-cpp-cmake32CircleCI Status
docker-spark-integrationCircleCI Status
docker-c_glibCircleCI Status
docker-rCircleCI Status
docker-rustCircleCI Status
docker-cppCircleCI Status
docker-python-3.6-alpineCircleCI Status
docker-python-2.7-nopandasCircleCI Status
docker-clang-formatCircleCI Status
docker-goCircleCI Status
docker-pandas-masterCircleCI Status
docker-python-3.7CircleCI Status
docker-lintCircleCI Status
docker-python-2.7CircleCI Status
docker-jsCircleCI Status
docker-python-2.7-alpineCircleCI Status
docker-javaCircleCI Status
docker-turbodbc-integrationCircleCI Status
docker-python-3.6-nopandasCircleCI Status
docker-docsCircleCI Status
docker-cpp-alpineCircleCI Status

conda's cmake3.2 does not support https for third party download.
- Ensure that LD_LIBRARY_PATH is set before installing R packages because
conda's pkg-config would pickup libicu from conda at compile but
LD_LIBRARY_PATH was not exported yet, thus package would fail tests.
- Fix JSON payload on systems where locales are missing.
@fsaintjacques
fsaintjacquesforce-pushed the ARROW-5631-cmake32-boost branch from 46d5da0 to 01f67ddCompareJune 21, 2019 19:14
@fsaintjacques

Copy link
Copy Markdown
ContributorAuthor

This fixes the most pressing issues, notably

  • docker-cpp
  • docker-cmake32

I fixed also fixed docker-rust, and partly advanced on docker-r and docker-c_glib.

@fsaintjacques

Copy link
Copy Markdown
ContributorAuthor

@ursabot crossbow test docker-cpp docker-cpp-cmake32

@ursabot

Copy link
Copy Markdown

AMD64 Conda Crossbow (#23443) builder has been succeeded.

Revision: 01f67dd

Submitted crossbow builds: ursa-labs/crossbow @ ursabot-47

TaskStatus
docker-cppCircleCI Status
docker-cpp-cmake32CircleCI Status

@koukou 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.

gandiva-glib/meson.build:50:0: ERROR: Tried to create target "enums.c", but a target of that name already exists.

This is a Meson bug. It has been fixed in the latest Meson. But the latest Meson (0.51.0) has a environment variable related bug: mesonbuild/meson#5503mesonbuild/meson#5502

So we need to use 0.50.1 for now.

Comment threadr/Dockerfile Outdated

# r-base includes tzdata. Get around interactive stop in that package
ENV DEBIAN_FRONTEND=noninteractive
# workaround for install_github Github API rate limit

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.

Github -> GitHub

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.

fixing

Comment threadc_glib/Dockerfile Outdated
ARROW_INSTALL_NAME_RPATH=OFF \
LD_LIBRARY_PATH="${CONDA_PREFIX}/lib" \
PKG_CONFIG_PATH="${CONDA_PREFIX}/lib/pkgconfig" \
PKG_CONFIG_PATH="${CONDA_PREFIX}/lib/pkgconfig:/usr/lib/x86_64-linux-gnu/pkgconfig" \

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.

Is this needed?
The added path is included in system pkg-config path. So it'll be searched without this.

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.

I'm going to revert this for now

@kou

kou commented Jun 21, 2019

Copy link
Copy Markdown
Member

BTW, I couldn't find any build log on CircleCI.

https://circleci.com/gh/ursa-labs/crossbow/tree/ursabot-46-circle-docker-c_glib shows

image

And "build" link shows

image

@wesmwesm 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.

+1. Made a couple of tweaks. Merging -- please open follow up JIRAs about other docker failures

Comment threadc_glib/Dockerfile Outdated
ARROW_INSTALL_NAME_RPATH=OFF \
LD_LIBRARY_PATH="${CONDA_PREFIX}/lib" \
PKG_CONFIG_PATH="${CONDA_PREFIX}/lib/pkgconfig" \
PKG_CONFIG_PATH="${CONDA_PREFIX}/lib/pkgconfig:/usr/lib/x86_64-linux-gnu/pkgconfig" \

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.

I'm going to revert this for now

Comment threadr/Dockerfile Outdated

# r-base includes tzdata. Get around interactive stop in that package
ENV DEBIAN_FRONTEND=noninteractive
# workaround for install_github Github API rate limit

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.

fixing

Sign up for freeto 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.

5 participants

@fsaintjacques@wesm@kou@codecov-io@ursabot