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

[CI] Add sirius lib to PATH #1725

Merged
merged 5 commits into from
Dec 27, 2023
Merged

Conversation

sylvlecl
Copy link
Member

Description

On windows, running antares requires to have sirius DLL in the search path for libraries.
In particular, unit tests linked with sirius need that.

The current implemented solution is a somewhat dirty workaround:
for each executable linked (possibly transitively) with sirius, we have a special CMake macro which needs to be used to copy sirius DLL to the same directory as the executable.

This requires developers to modify the build just to solve an installation issue, and this for each executable that are linked with sirius.

Solution
Instead, the PR proposes to add sirius installation directory to the PATH variable.
Executing unit tests will now rely on normal windows DLL search mechanism, and the developer only has to use the standard target_link_libraries in CMake files.

@sonarqubecloud
Copy link

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 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@a-zakir
Copy link
Contributor

a-zakir commented Oct 31, 2023

i personally use CMAKE_PREFIX_PATH for sirius and ortools install directory, i think it's all the same 👍🏽

@sylvlecl
Copy link
Member Author

sylvlecl commented Nov 2, 2023

i personally use CMAKE_PREFIX_PATH for sirius and ortools install directory, i think it's all the same 👍🏽

Ah, thanks! it sound better, if it works, it does not require the user to alter the environment outside of cmake related commands and files.
In that case, the best thing would be to remove completely DEPS_INSTALL_DIR from expected variables. It has no reason to exist in the first place in my opinion, we should have used CMAKE_PREFIX_PATH for this, it's the standard way to define dependencies directories.

@sylvlecl
Copy link
Member Author

sylvlecl commented Nov 3, 2023

Unfortunately, only CMAKE_PREFIX_PATH is not enough to make the execution work.
The DLL is still not found when executing test, see that run on a test branch:
https://github.com/AntaresSimulatorTeam/Antares_Simulator/actions/runs/6737147648

The following tests FAILED:
	  5 - end-to-end-simple-study (Exit code 0xc0000135
)
	  6 - end-to-end-binding_constraints (Exit code 0xc0000135
)

So it seems that it's necessary to add it in the PATH.

@a-zakir
Copy link
Contributor

a-zakir commented Nov 3, 2023

Unfortunately, only CMAKE_PREFIX_PATH is not enough to make the execution work. The DLL is still not found when executing test, see that run on a test branch: https://github.com/AntaresSimulatorTeam/Antares_Simulator/actions/runs/6737147648

The following tests FAILED:
	  5 - end-to-end-simple-study (Exit code 0xc0000135
)
	  6 - end-to-end-binding_constraints (Exit code 0xc0000135
)

So it seems that it's necessary to add it in the PATH.

🤐

@JasonMarechal25
Copy link
Contributor

Solution Instead, the PR proposes to add sirius installation directory to the PATH variable. Executing unit tests will now rely on normal windows DLL search mechanism, and the developer only has to use the standard target_link_libraries in CMake files.

Is the solution described up to date? The code change does not explicitly refer to target_link_library

Also, does it impact local development in any way? Looking only at the changelog I would say yes because dependency is not copied anymore but maybe it supported by other configuration?

@sylvlecl
Copy link
Member Author

sylvlecl commented Nov 3, 2023

Is the solution described up to date? The code change does not explicitly refer to target_link_library

Yes it is: the goal of the change is that using target_link_libraries is enough, we don't need anymore to use copy_dependency.

Also, does it impact local development in any way? Looking only at the changelog I would say yes because dependency is not copied anymore but maybe it supported by other configuration?

Indeed it does: the developer must ensure to have sirius in its PATH too (or any other place that windows searches for).
The developer must ensure his setup is correct for the OS he uses.
I will check where we could add some documentation for it.

For me it does not invalidate the solution: it does not make sense to copy libs we want to use in all directories where we want to execute something. The goal of shared libraries is to be shared from the one place where they are installed :)

@a-zakir
Copy link
Contributor

a-zakir commented Nov 28, 2023

I’m takin it

Watermelon AI Summary

AI Summary deactivated by sylvlecl

GitHub PRs

Antares_Simulator is an open repo and Watermelon will serve it for free.
🍉🫶
Have you starred Watermelon?

@flomnes flomnes merged commit 7590941 into develop Dec 27, 2023
4 checks passed
@flomnes flomnes deleted the fix/path-to-sirius-for-unit-tests branch December 27, 2023 14:44
Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

flomnes added a commit that referenced this pull request Feb 6, 2024
**Description**

On windows, running antares requires to have sirius DLL in the search
path for libraries.
In particular, unit tests linked with sirius need that.

The current implemented solution is a somewhat dirty workaround:
for each executable linked (possibly transitively) with sirius, we have
a special CMake macro which needs to be used to copy sirius DLL to the
same directory as the executable.

This requires developers to modify the build just to solve an
installation issue, and this for **each** executable that are linked
with sirius.

**Solution**
Instead, the PR proposes to add sirius installation directory to the
PATH variable.
Executing unit tests will now rely on normal windows DLL search
mechanism, and the developer only has to use the standard
`target_link_libraries` in CMake files.

---------

Signed-off-by: Sylvain Leclerc <[email protected]>
Co-authored-by: Florian OMNES <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants