Change cmake config to use gcc #76

volpatto · 2019-10-12T22:11:39Z

This PR changes the C and C++ compilers to use gcc family instead clang as required in #70.

Remarks:

I changed the code generation in hookman/hookman_generator.py, please see lines 592-598 there.
Instead installing clang-dev from conda-forge, now compilers is required (as we need both C and C++ compilers).
One of the tests is failing. The error is related to pytest-datadir and file regression. I have no previous experiences with this plugin and I have been facing some struggle with it. I need some help in this regard.

Thanks in advance.

tadeu · 2019-10-14T13:25:43Z

build/CMakeLists.txt

@@ -6,8 +6,8 @@ set(PROJECT_NAME hookman)
 set(ARTIFACTS_DIR ${CMAKE_CURRENT_SOURCE_DIR}/artifacts)

 if(NOT WIN32)
-  set(CMAKE_C_COMPILER    clang)
-  set(CMAKE_CXX_COMPILER  clang++)
+  set(CMAKE_C_COMPILER    cc)


What happens if these 4 lines are removed, and we leave the defaults? (If the result is the same, it's better to remove the lines)

I can try it. You mean, remove the whole if?

environment.devenv.yml

tadeu · 2019-10-14T13:26:37Z

hookman/hookman_generator.py

@@ -589,13 +589,13 @@ def _plugin_cmake_file_content(self, plugin_id):
            set(ARTIFACTS_DIR ${{CMAKE_CURRENT_SOURCE_DIR}}/artifacts)

            if(NOT WIN32)
-              set(CMAKE_C_COMPILER    clang)
-              set(CMAKE_CXX_COMPILER  clang++)
+              set(CMAKE_C_COMPILER    cc)


Could these two lines be removed?

I will try.

tadeu · 2019-10-14T13:27:17Z

Is the fail also on master? I'll ask @williamjamir to take a look on this, then

williamjamir · 2019-10-14T14:10:35Z

First of all, thanks for to take a look on this issue =)

To solve the issue, you need to re-run the test (locally) with the --force-regen parameter ;)

williamjamir · 2019-10-14T14:12:15Z

Is the fail also on master? I'll ask @williamjamir to take a look on this, then

Currently, only appveyor on python 3.7 is failing.

I'm hoping to fix this issue by migrating the CI to Github actions ;)

arthursoprana

Please take a look at the comments

build/HookManConfig.cmake

environment.devenv.yml

codecov · 2019-10-14T15:37:57Z

Codecov Report

Merging #76 into master will decrease coverage by 1.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master      #76      +/-   ##
==========================================
- Coverage   99.57%   98.52%   -1.06%     
==========================================
  Files           7        7              
  Lines         474      474              
==========================================
- Hits          472      467       -5     
- Misses          2        7       +5

.travis.yml

volpatto · 2019-10-14T15:42:59Z

First of all, thanks for to take a look on this issue =)

To solve the issue, you need to re-run the test (locally) with the --force-regen parameter ;)

Thanks, @williamjamir! Now all tests are passing!!

volpatto · 2019-10-15T18:17:01Z

environment.devenv.yml

+    - compilers
+    - gxx_impl_linux-64
+    - gcc_impl_linux-64
+    - gxx_linux-64
+    - gcc_linux-64


@tadeu @arthursoprana I need some help here, please. The thing is: conda env breaks if we use compilers only. Then I took a look at eden and included here a similar stack for C/Cpp. However, in Travis, the compilers are gathered from the system instead of the env. Please take a look at the snippet below:

-- The CXX compiler identification is GNU 5.4.0 -- The C compiler identification is GNU 5.4.0 -- Check for working CXX compiler: /usr/bin/c++ -- Check for working CXX compiler: /usr/bin/c++ -- works -- Detecting CXX compiler ABI info -- Detecting CXX compiler ABI info - done -- Detecting CXX compile features -- Detecting CXX compile features - done -- Check for working C compiler: /usr/bin/cc -- Check for working C compiler: /usr/bin/cc -- works

Is this OK? What do you recommend?

No, it should use the compiler from the environment. Probably the line set(CMAKE_CXX_COMPILER c++) (etc.) is messing up, it's trying to find c++ instead of what conda defines in the CXX env var (and the CC env var).

The GCC version should be 7.3.0 (this 5.4.0 is from some compiler installed in the CI system)

Ok, I will try to seek a way to fix this problem

@tadeu fixed. The thing is: the CI test is performed within an env created by tox. However, conda_deps inside tox.ini doesn't include C/C++ compilers according to conda. So, inside tox env, there is no compiler, therefore the compilers will be gathered from system. I had to change tox.ini to include a minimal set of compilers for C and C++. I'm aware about compilers from conda, they are intend to be used for builds, but I think that we are OK using such package for tests with tox here.

volpatto · 2019-10-17T18:53:52Z

tox.ini

+    conda-forge::gcc_linux-64==7.3.0
+    conda-forge::gcc_impl_linux-64==7.3.0
+    conda-forge::gxx_linux-64==7.3.0
+    conda-forge::gxx_impl_linux-64==7.3.0
+    libgcc-ng==7.3.0
+    libstdcxx-ng==7.3.0
+    binutils_linux-64==2.31.1
+    binutils_impl_linux-64==2.31.1


Great, thanks a lot for the help! Don't you think that would be interesting if we use >= to keep track of updates in the compilers?

Yes, let's see first if this works first, its just a guess :)
If it works, we change to >=

tadeu · 2019-10-17T19:00:00Z

Meh, it didn't work, it's probably because of tox, it probably is installing the conda packages but isn't "activating" them, so environment variables such as CXX and CC won't be defined.

Maybe we should change to use conda devenv and invoke, instead of tox? What do you think @nicoddemus and @williamjamir?

nicoddemus · 2019-10-17T19:24:37Z

Meh, it didn't work, it's probably because of tox, it probably is installing the conda packages but isn't "activating" them, so environment variables such as CXX and CC won't be defined.

That can be checked by adding a echo $CXX as the first line in commands.

Maybe we should change to use conda devenv and invoke, instead of tox? What do you think @nicoddemus and @williamjamir?

I'm fine with it. 👍

tadeu · 2019-10-17T20:34:08Z

Ok, but I think that changing from tox go beyond from what @volpatto was trying to do here. @volpatto, perhaps, for now, you can try tp just update Travis configuration so that it uses a newer GCC (>= 7.3.0 instead of 5.4.0), and drop these compilers from tox (since they aren't being used anyway)

volpatto · 2019-10-21T13:44:14Z

Ok, but I think that changing from tox go beyond from what @volpatto was trying to do here. @volpatto, perhaps, for now, you can try tp just update Travis configuration so that it uses a newer GCC (>= 7.3.0 instead of 5.4.0), and drop these compilers from tox (since they aren't being used anyway)

Nice, then. Maybe we can use the "compiler set for build" (compilers and gxx_linux things) from conda, since this way hookman's build is actually using the recent compilers from conda env. I don't see it as a solution, since the intention of those packages are to supply compiler resources for package buildings (AFAIK). But it can be a fix for now.

What do you think, @tadeu @nicoddemus @arthursoprana @williamjamir? Or it would be better to forget it and just set it in Travis (just as @tadeu said)?

volpatto · 2019-10-21T14:36:05Z

Ok, but I think that changing from tox go beyond from what @volpatto was trying to do here. @volpatto, perhaps, for now, you can try tp just update Travis configuration so that it uses a newer GCC (>= 7.3.0 instead of 5.4.0), and drop these compilers from tox (since they aren't being used anyway)

Nice, then. Maybe we can use the "compiler set for build" (compilers and gxx_linux things) from conda, since this way hookman's build is actually using the recent compilers from conda env. I don't see it as a solution, since the intention of those packages are to supply compiler resources for package buildings (AFAIK). But it can be a fix for now.

What do you think, @tadeu @nicoddemus @arthursoprana @williamjamir? Or it would be better to forget it and just set it in Travis (just as @tadeu said)?

Builds OK, except for appveyor, but this is expected (and we hope that it will be fixed when we move to GH Actions). Please note that the compilers are properly used from tox env with the current tox.ini.

volpatto · 2020-05-22T11:58:00Z

A kind ping here, since @tadeu approved recently this PR and I don't know what to do at the moment.

Cheers!

arthursoprana

I guess if this is passing you may merge it ;)

But appveyor is failing...

williamjamir · 2020-05-22T23:48:18Z

The appVeyor is broken with the following message:

Solving environment: ...working... failed
PackagesNotFoundError: The following packages are not available from current channels:
  - conda-forge::gcc_impl_linux-64==7.3.0
  - conda-forge::gxx_impl_linux-64==7.3.0
  - conda-forge::gxx_linux-64==7.3.0
  - conda-forge::gcc_linux-64==7.3.0
Current channels:

volpatto · 2020-06-05T13:59:40Z

As soon as I have some time, I'll come back here.

Perhaps the channel name conda-forge is not appropriated. Is hookman using ESSS mirrors?

volpatto · 2020-06-05T14:02:13Z

environment.devenv.yml

+    - conda-forge::gcc_linux-64==7.3.0
+    - conda-forge::gcc_impl_linux-64==7.3.0
+    - conda-forge::gxx_linux-64==7.3.0
+    - conda-forge::gxx_impl_linux-64==7.3.0


These packages cannot be found at the moment. Further investigation is required.

volpatto · 2020-06-05T15:08:29Z

As soon as I have some time, I'll come back here.

Perhaps the channel name conda-forge is not appropriated. Is hookman using ESSS mirrors?

Dumb question... it's a publicly available ESSS package. So I should check compiler-related package versions.

nicoddemus · 2022-09-05T18:20:01Z

Is this still relevant or can we close this?

volpatto · 2022-09-05T18:45:41Z

Is this still relevant or can we close this?

I think this is a really old PR, and don't even know if this is still worthy. IMHO, we can decline it, @nicoddemus. If we need something as we have in this PR, it is easy to redo.

nicoddemus · 2022-09-05T18:49:01Z

Can you delete it then please @volpatto ?

Ask Bender:

!delete-branch change-cmake-config-to-use-gcc

nicoddemus · 2022-09-05T18:50:00Z

Actually I just realized you probably only created this branch in this repository right? Then just close it here please. 👍

volpatto added 3 commits October 12, 2019 18:17

Change build to use gcc instead clang

bc2ff60

Change cxx-compiler and c-compiler to compilers

76d529c

Change code generator to gcc compilers family

96e3b63

volpatto requested review from tadeu, nicoddemus, arthursoprana and williamjamir October 12, 2019 22:11

nicoddemus approved these changes Oct 14, 2019

View reviewed changes

tadeu reviewed Oct 14, 2019

View reviewed changes

arthursoprana suggested changes Oct 14, 2019

View reviewed changes

build/HookManConfig.cmake Outdated Show resolved Hide resolved

environment.devenv.yml Outdated Show resolved Hide resolved

Diego Volpatto added 2 commits October 14, 2019 12:34

Remove cython as dependency and fix datadir related error

150f64d

Remove llvm and clang from travis yml

75c8bd5

volpatto commented Oct 14, 2019

View reviewed changes

.travis.yml Show resolved Hide resolved

Diego Volpatto added 3 commits October 14, 2019 12:44

Set CXX standard with cmake

f99f867

Set CXX standard with cmake (now inside hookman generator)

0977d14

Fix datadir according to cmake generator changes

c28123d

volpatto requested review from arthursoprana and tadeu October 14, 2019 18:36

williamjamir approved these changes Oct 14, 2019

View reviewed changes

Diego Volpatto added 3 commits October 15, 2019 11:26

Remove hard coded C compiler setup in cmake related parts

9a450bf

Test a change for compilers in the env

256fb53

Set gcc compilers in an eden similar fashion

e3144be

volpatto commented Oct 15, 2019

View reviewed changes

Diego Volpatto added 2 commits October 16, 2019 16:10

Test to re-include some compiler setup for cmake

0227196

Restore compilers setup: getting from standard

a4f4485

Diego Volpatto and others added 6 commits October 16, 2019 16:26

[WIP] Quick test

ab2489c

[WIP] Undo

11508b2

[WIP] Another try

b634a45

Add more dependencies needed by compilers

e2bc830

Add compilers explicitly in tox.ini

eeade47

Add conda-forge channel back

f3ce52a

volpatto commented Oct 17, 2019

View reviewed changes

[WIP] Try a small change in tox for compilers

ef6d56c

tadeu approved these changes May 20, 2020

View reviewed changes

arthursoprana approved these changes May 22, 2020

View reviewed changes

volpatto commented Jun 5, 2020

View reviewed changes

volpatto closed this Sep 5, 2022

volpatto deleted the change-cmake-config-to-use-gcc branch September 5, 2022 18:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change cmake config to use gcc #76

Change cmake config to use gcc #76

volpatto commented Oct 12, 2019

tadeu Oct 14, 2019

volpatto Oct 14, 2019

tadeu Oct 15, 2019

tadeu Oct 14, 2019

volpatto Oct 14, 2019

tadeu commented Oct 14, 2019

williamjamir commented Oct 14, 2019

williamjamir commented Oct 14, 2019

arthursoprana left a comment

codecov bot commented Oct 14, 2019 •

edited

Loading

volpatto commented Oct 14, 2019

volpatto Oct 15, 2019 •

edited

Loading

tadeu Oct 15, 2019 •

edited

Loading

volpatto Oct 16, 2019

volpatto Oct 16, 2019

volpatto Oct 17, 2019 •

edited

Loading

tadeu Oct 17, 2019

tadeu commented Oct 17, 2019

nicoddemus commented Oct 17, 2019

tadeu commented Oct 17, 2019

volpatto commented Oct 21, 2019 •

edited

Loading

volpatto commented Oct 21, 2019

volpatto commented May 22, 2020

arthursoprana left a comment •

edited

Loading

williamjamir commented May 22, 2020

volpatto commented Jun 5, 2020

volpatto Jun 5, 2020

volpatto commented Jun 5, 2020

nicoddemus commented Sep 5, 2022

volpatto commented Sep 5, 2022

nicoddemus commented Sep 5, 2022 •

edited

Loading

nicoddemus commented Sep 5, 2022

Change cmake config to use gcc #76

Change cmake config to use gcc #76

Conversation

volpatto commented Oct 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tadeu commented Oct 14, 2019

williamjamir commented Oct 14, 2019

williamjamir commented Oct 14, 2019

arthursoprana left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 14, 2019 • edited Loading

Codecov Report

volpatto commented Oct 14, 2019

volpatto Oct 15, 2019 • edited Loading

Choose a reason for hiding this comment

tadeu Oct 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

volpatto Oct 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tadeu commented Oct 17, 2019

nicoddemus commented Oct 17, 2019

tadeu commented Oct 17, 2019

volpatto commented Oct 21, 2019 • edited Loading

volpatto commented Oct 21, 2019

volpatto commented May 22, 2020

arthursoprana left a comment • edited Loading

Choose a reason for hiding this comment

williamjamir commented May 22, 2020

volpatto commented Jun 5, 2020

Choose a reason for hiding this comment

volpatto commented Jun 5, 2020

nicoddemus commented Sep 5, 2022

volpatto commented Sep 5, 2022

nicoddemus commented Sep 5, 2022 • edited Loading

nicoddemus commented Sep 5, 2022

codecov bot commented Oct 14, 2019 •

edited

Loading

volpatto Oct 15, 2019 •

edited

Loading

tadeu Oct 15, 2019 •

edited

Loading

volpatto Oct 17, 2019 •

edited

Loading

volpatto commented Oct 21, 2019 •

edited

Loading

arthursoprana left a comment •

edited

Loading

nicoddemus commented Sep 5, 2022 •

edited

Loading