Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/remove enable if #282

Merged

Conversation

This reverts commit 03bec69.

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
This reverts commit e5a81fd.

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers mgovers added the improvement Improvement on internal implementation label Jun 26, 2023
@mgovers mgovers changed the base branch from main to release/short-circuit-calculation June 26, 2023 07:12
mgovers and others added 13 commits June 26, 2023 10:16
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers mgovers marked this pull request as ready for review June 26, 2023 11:21
@TonyXiang8787
Copy link
Member

@mgovers please have a look at the build guide. To see if the required C++ standard and compiler version needs to be updated.

@mgovers
Copy link
Member Author

mgovers commented Jun 26, 2023

@mgovers please have a look at the build guide. To see if the required C++ standard and compiler version needs to be updated.

According to the build guide:

Compiler Support

You need a C++ compiler with C++20 support. Below is a list of tested compilers:

Linux

  • gcc >= 10.0
    • Version 10.0 tested using the version in the manylinux2014 container.
    • Version 11.x tested in CI
  • Clang >= 14.0
    • Version 14.x tested in CI

You can define the environment variable CXX to for example clang++ to specify the C++ compiler.

Windows

  • MSVC >= 17.5
    • Latest release tested in CI (e.g. Visual Studio 2022, IDE or build tools)
  • Clang CL >= 14.0
    • Latest release tested in CI (e.g. Visual Studio 2022, IDE or build tools)

macOS

  • Clang >= 14.0
    • Latest release tested in CI

According to https://en.cppreference.com/w/cpp/compiler_support/20 , GCC has full support for concepts starting with GCC 10 and onward, so in that sense we shouldn't have to update the docs. However, the build guide still refers to manylinux2014. I will bump that to manylinux_2_28 and also bump the GCC version to 11.2 (which is the version in the manylinux_2_28 container) and 12.2 tested using the custom musllinux build

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@TonyXiang8787
Copy link
Member

@mgovers

manylinux_2_28 has GCC 12.
https://github.com/pypa/manylinux

@mgovers
Copy link
Member Author

mgovers commented Jun 26, 2023

@mgovers

manylinux_2_28 has GCC 12.
https://github.com/pypa/manylinux

oh sorry apparently i was looking at an older thread (pypa/manylinux#1282). will fix. should we also bump the gcc version in CI?

@TonyXiang8787
Copy link
Member

@mgovers
manylinux_2_28 has GCC 12.
https://github.com/pypa/manylinux

oh sorry apparently i was looking at an older thread (pypa/manylinux#1282). will fix. should we also bump the gcc version in CI?

You mean in the documentation or in CI itself?

@mgovers
Copy link
Member Author

mgovers commented Jun 26, 2023

@mgovers
manylinux_2_28 has GCC 12.
https://github.com/pypa/manylinux

oh sorry apparently i was looking at an older thread (pypa/manylinux#1282). will fix. should we also bump the gcc version in CI?

You mean in the documentation or in CI itself?

CI itself, i mean. i'm fine either way but want to double-check

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@TonyXiang8787
Copy link
Member

@mgovers
manylinux_2_28 has GCC 12.
https://github.com/pypa/manylinux

oh sorry apparently i was looking at an older thread (pypa/manylinux#1282). will fix. should we also bump the gcc version in CI?

You mean in the documentation or in CI itself?

CI itself, i mean. i'm fine either way but want to double-check

I think it is good to update the CI compiler version to the newest.

@TonyXiang8787
Copy link
Member

There is a work around to counter a GCC 10 bug.

// TODO, remove this separate definition for UpdateType after migrating to gcc-11

Since we upgrade to GCC 12, that work around can be removed.

@mgovers
Copy link
Member Author

mgovers commented Jun 26, 2023

now that i think about it, we'd better bump CI compiler versions in a separate PR because it may cause other not-yet-seen issues and i would like to isolate changes

@TonyXiang8787
Copy link
Member

now that i think about it, we'd better bump CI compiler versions in a separate PR because it may cause other not-yet-seen issues and i would like to isolate changes

Okay. After you add comment for rk2_tensor. I will approve it.

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@sonarcloud
Copy link

sonarcloud bot commented Jun 26, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@TonyXiang8787 TonyXiang8787 merged commit e93174d into release/short-circuit-calculation Jun 26, 2023
28 checks passed
@TonyXiang8787 TonyXiang8787 deleted the feature/remove-enable-if branch June 26, 2023 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement on internal implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants