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

Removed version number from Modelica code #64

Merged
merged 2 commits into from
Jun 27, 2022

Conversation

jowr
Copy link
Collaborator

@jowr jowr commented Jun 19, 2022

Removed version number from Modelica code and updated the version information to v3.3.2

Closes #63

@jowr jowr requested a review from casella June 19, 2022 15:13
@casella
Copy link
Collaborator

casella commented Jun 22, 2022

@jowr, your changes look good to me.

I see the CI script is failing because it cannot link (statically?) _ModelicaError and _ModelicaWarning. As I understand, these symbol should be loaded dynamically at runtime. Maybe some macros were not defined correctly, so that some bits and pieces of the statically linked version are still compiled?

Adding @fedetftpolimi to the loop.

Copy link
Collaborator

@casella casella left a comment

Choose a reason for hiding this comment

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

Changes to the code LGTM

@jowr
Copy link
Collaborator Author

jowr commented Jun 23, 2022

Please note that CI only fails on macOS only. I am not even sure that this is among our target platforms.

@casella
Copy link
Collaborator

casella commented Jun 23, 2022

Please note that CI only fails on macOS only. I am not even sure that this is among our target platforms.

As far as I am personally concerned, it isn't. Our experience with OpenModelica is that keeping MacOS compatibility is very hard, because MacOS changes all the time, often in non-backwards compatible ways, and it requires significant resources to keep up with it.

I would suggest to suspend MacOS support for the time being and make sure we are getting full support with Dymola and OpenModelica under Windows and Linux. That is already a major milestone, and I would really like to finalize it ASAP. Later on, we can try to add even more Modelica tools and operating systems. I'll open two tickets about them, so people can add their request of support for them.

@fedetftpolimi fedetftpolimi merged commit 5a8d0de into dynamiclib Jun 27, 2022
@casella
Copy link
Collaborator

casella commented Jun 27, 2022

@jowr we merged your PR into the dynamiclib branch. We still need to fix the issue of where the compiled libraries are copied, see #66. @fedetftpolimi should provide you indications about the locations on Linux ASAP.

Thanks!

@fedetftpolimi
Copy link
Collaborator

The dynamiclib branch merged the feature/no_version_number branch, which is likely no longer required.
In the dynamiclib branch I also just made a commit that fixes the CMake install target, it still installed the library in the directory with the version number, creating it in the process. Now libraries are installed in the directory without version number.

@beutlich beutlich deleted the feature/no_version_number branch July 5, 2022 18:18
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.

None yet

3 participants