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

Update build_tarballs_release.jl #78

Merged
merged 1 commit into from
Nov 15, 2024
Merged

Conversation

amontoison
Copy link
Contributor

Compile Uno with HiGHS for future releases.

Compile Uno with HiGHS for future releases.
@@ -136,6 +137,7 @@ products = [

# Dependencies that must be installed before this package can be built
dependencies = [
Dependency(PackageSpec(name="HiGHS_jll", uuid="8fd58aa0-07eb-5a78-9b36-339c94fd15ea")),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Dependency(PackageSpec(name="HiGHS_jll", uuid="8fd58aa0-07eb-5a78-9b36-339c94fd15ea")),
Dependency(PackageSpec(name="HiGHS_jll", uuid="8fd58aa0-07eb-5a78-9b36-339c94fd15ea"), compat="1"),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@odow It's not needed in that case because we don't use it in the Julia ecosystem, so the shared libraries can't be automatically upgraded and replaced.

If the CI build with Julia tests passed, it's safe to generate binaries for a use outside Julia when a new release is tagged.

@odow
Copy link
Contributor

odow commented Nov 7, 2024

Maybe we should wait until #77?

I'd like to know that HiGHS actually works in Uno before merging. Ideally, we would have done the tests and CI changes in the PR that added HiGHS in the first place.

@amontoison
Copy link
Contributor Author

amontoison commented Nov 7, 2024

This issue is that we already merged the PR #75, which is the one that tests Uno_jll.jl in the Julia ecosystem.
This PR is just the releases of Uno.

@odow
Copy link
Contributor

odow commented Nov 7, 2024

I know. But currently #75 isn't available to the wider world. If we merge this then the next time that Charlie tags a release, HiGHS will be included. But I think we need to fix HiGHS actually passing tests before we include it in the public releases.

@cvanaret cvanaret merged commit babd927 into cvanaret:main Nov 15, 2024
3 checks passed
@cvanaret
Copy link
Owner

cvanaret commented Nov 15, 2024

The build of v1.3.0 (the x86_64-w64-mingw32-libgfortran5 artifact) failed with the following message:

[13:34:26] /opt/x86_64-w64-mingw32/bin/../lib/gcc/x86_64-w64-mingw32/10.2.0/../../../../x86_64-w64-mingw32/bin/ld: d000003.o:(.idata$2+0x0): multiple definition of `_head_libhighs_dll'; /opt/x86_64-w64-mingw32/x86_64-w64-mingw32/sys-root/usr/local/lib/libhighs.dll.a(d000134.o):(.idata$2+0x0): first defined here
[13:34:26] /opt/x86_64-w64-mingw32/bin/../lib/gcc/x86_64-w64-mingw32/10.2.0/../../../../x86_64-w64-mingw32/bin/ld: d000004.o:(.idata$5+0x0): multiple definition of `__imp__ZTV17PresolveComponent'; /opt/x86_64-w64-mingw32/x86_64-w64-mingw32/sys-root/usr/local/lib/libhighs.dll.a(d003552.o):(.idata$5+0x0): first defined here
[13:34:26] /opt/x86_64-w64-mingw32/bin/../lib/gcc/x86_64-w64-mingw32/10.2.0/../../../../x86_64-w64-mingw32/bin/ld: d000004.o:(.idata$6+0x0): multiple definition of `__nm__ZTV17PresolveComponent'; /opt/x86_64-w64-mingw32/x86_64-w64-mingw32/sys-root/usr/local/lib/libhighs.dll.a(d003552.o):(.idata$6+0x0): first defined here
[13:34:26] /opt/x86_64-w64-mingw32/bin/../lib/gcc/x86_64-w64-mingw32/10.2.0/../../../../x86_64-w64-mingw32/bin/ld: d000005.o:(.idata$7+0x0): multiple definition of `libhighs_dll_iname'; /opt/x86_64-w64-mingw32/x86_64-w64-mingw32/sys-root/usr/local/lib/libhighs.dll.a(d003778.o):(.idata$7+0x0): first defined here
[13:34:26] collect2: error: ld returned 1 exit status

Everything went fine during CI. Do you understand what happened?

@amontoison
Copy link
Contributor Author

On Windows, an additional file is generated: libuno.dll.a.
This is an "import library," which helps link against the shared library libuno.dll.
It contains references to the symbols exported by the DLL and ensures compatibility with compilers like MSVC, which require this intermediate file.

We should try to compile with an older version of HiGHS.

We don't need it when using gcc directly, but it is mandatory for projects compiled with MSVC if they rely on the DLL.

@amontoison
Copy link
Contributor Author

I quickly investigated, and it seems that the linker on Windows is using both libuno.dll.a and libuno.dll. I should try locally to see if removing libuno.dll.a before compilation fixes the problem.
It's always a pleasure to compile code on Windows...

@cvanaret
Copy link
Owner

cvanaret commented Nov 15, 2024

It's always a pleasure to compile code on Windows...

Ahaha I'm sorry to put thankless tasks on you like that :(
Bad news: now the next cross-compilation (x86_64-apple-darwin-cxx11) fails with:

[19:49:32] /workspace/srcdir/Uno/uno/options/Presets.cpp:17:48: error: 'value' is unavailable: introduced in macOS 10.14
[19:49:32]    17 |          Presets::set(options, optional_preset.value());
[19:49:32]       |                                                ^
[19:49:32] /opt/x86_64-apple-darwin14/x86_64-apple-darwin14/sys-root/usr/include/c++/v1/optional:938:33: note: 'value' has been explicitly marked unavailable here
[19:49:32]   938 |     constexpr value_type const& value() const&
[19:49:32]       |                                 ^
[19:49:32] 1 error generated.

although it's available in the C++17 standard: https://en.cppreference.com/w/cpp/utility/optional/value
How easy is it to switch to >= macOS 10.14?

@amontoison
Copy link
Contributor Author

@cvanaret
You should add CI tests on Windows and Mac.
I will investigate but it's so easier to keep track of these things and do not wait a new release to discover that the compilation is broken on platforms other than Linux.

@amontoison
Copy link
Contributor Author

Do you use the option -std=c++17 during compilation?
Otherwise, we probably need something like -mmacosx-version-min=10.14.

@amontoison
Copy link
Contributor Author

amontoison commented Nov 15, 2024

@cvanaret
You seem to use -std=gnu++17 by default I don't know if all features of C++ are available for that with clang++:

/opt/bin/x86_64-apple-darwin14-libgfortran5-cxx11/x86_64-apple-darwin14-clang++ --sysroot=/opt/x86_64-apple-darwin14/x86_64-apple-darwin14/sys-root  -I/workspace/srcdir/Uno/uno -I/workspace/srcdir/Uno/build/include -I/workspace/destdir/lib -I/opt/x86_64-apple-darwin14/x86_64-apple-darwin14/sys-root/usr/local/lib -isystem /opt/x86_64-apple-darwin14/x86_64-apple-darwin14/sys-root/usr/local/include/highs -Wall -Wextra -Wnon-virtual-dtor -pedantic -Wunused-value -Wconversion -O3 -DNDEBUG -mmacosx-version-min=10.10 -fPIC   -D HAS_HSL -D HAS_METIS -D HAS_HIGHS -D MUMPS_SEQUENTIAL -D HAS_MUMPS_MPISEQ_LIBRARY -D HAS_MUMPS -D HAS_AMPLSOLVER -fopenmp=libomp -std=gnu++17 -MD -MT CMakeFiles/uno.dir/uno/preprocessing/Scaling.cpp.o -MF CMakeFiles/uno.dir/uno/preprocessing/Scaling.cpp.o.d -o CMakeFiles/uno.dir/uno/preprocessing/Scaling.cpp.o -c /workspace/srcdir/Uno/uno/preprocessing/Scaling.cpp

@cvanaret
Copy link
Owner

@cvanaret You should add CI tests on Windows and Mac

I agree!

@cvanaret You seem to use -std=gnu++17 by default I don't know if all features of C++ are available for that with clang++:

Clang++ compiles on my Linux Mint with -std=gnu++17. I'll try with -mmacosx-version-min=10.14.

@amontoison
Copy link
Contributor Author

amontoison commented Nov 16, 2024

I found this related code of the build_tarballs.jl of SHOT:
https://github.com/JuliaPackaging/Yggdrasil/blob/daf1aea8a02a5de9089cb391bf915492214ec483/C/Coin-OR/SHOT/build_tarballs.jl#L18-L32

@cvanaret Where can I add -mmacosx-version-min=10.14 or replace -std=gnu++17 by -std=c++17 in the build system of Uno?
I can quickly try them in the interative mode of the cross-compiler.

@cvanaret
Copy link
Owner

cvanaret commented Nov 16, 2024

@amontoison can you try to add:

if (CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang")
   set(MACOSX_DEPLOYMENT_TARGET "10.14" CACHE STRING "Minimum OS X deployment version" FORCE)
endif()

somewhere here: https://github.com/cvanaret/Uno/blob/main/CMakeLists.txt#L8?

@cvanaret
Copy link
Owner

I started a PR draft here (#95) but at the moment, I'm not skilled enough :) The Fortran compiler cannot be detected.

@amontoison
Copy link
Contributor Author

@amontoison can you try to add:

if (CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang")
   set(MACOSX_DEPLOYMENT_TARGET "10.14" CACHE STRING "Minimum OS X deployment version" FORCE)
endif()

somewhere here: https://github.com/cvanaret/Uno/blob/main/CMakeLists.txt#L8?

I tried the option and I don't see any difference with or without it.

@cvanaret
Copy link
Owner

Fixed the std::optional issue with macOS in #101.
Added CI builds on Windows and macOS in #95.

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.

3 participants