modelica / ModelicaStandardLibrary

Free (standard conforming) library to model mechanical (1D/3D), electrical (analog, digital, machines), magnetic, thermal, fluid, control systems and hierarchical state machines. Also numerical functions and functions for strings, files and streams are included.
https://doc.modelica.org
BSD 3-Clause "New" or "Revised" License
453 stars 165 forks source link

Revert "Fix position prescribed friction" #4255

Closed MartinOtter closed 6 months ago

MartinOtter commented 6 months ago

Reverts modelica/ModelicaStandardLibrary#4129

When simulating a model with the changed friction model the following error appears in Dymola 2024x:

Compiling and linking the model (Visual C++).

dsmodel.c dsmodel.c(176): error C2065: "ModelicaStandardTables_CombiTable1D_init3": nichtdeklarierter Bezeichner dsmodel.c(176): error C2064: Ausdruck ergibt keine Funktion, die 337 Argumente bernimmt

Error generating Dymosim.

tobolar commented 6 months ago

Duplicate to #3911

tobolar commented 6 months ago

@beutlich Is it documented how to proceed in such cases or where to find up-to-date binaries? https://github.com/modelica/ModelicaStandardLibrary/issues/4178#issuecomment-1673710607 is well and good but not that easy to find it.

beutlich commented 6 months ago

Is it documented how to proceed in such cases or where to find up-to-date binaries?

They are usually updated by a library officer before tagging a release. See #4250.

beutlich commented 6 months ago

I wonder why this PR was merged bypassing the review process. This is not ideal. 😕

tobolar commented 6 months ago

Is it documented how to proceed in such cases or where to find up-to-date binaries?

They are usually updated by a library officer before tagging a release. See #4250.

Is there any procedure how to handle this before the binaries are updated? What about to e.g. handle PRs regarding binaries' changes differently to common PRs? (This is just a spontaneous idea.)

beutlich commented 6 months ago

Is there any procedure how to handle this

We had two options so far: You are always free to compile the C-Sources your own (targeting your OS and simulation tool). And: The library officers usally provided them upon request, see your reference to #4178.

Of course, we could also update the CI actions to build and deploy the binaries.

Edit: Please move this discussion to #4250 or a separate discussion.

MartinOtter commented 6 months ago

I wonder why this PR was merged bypassing the review process. This is not ideal. 😕

Because "external_c_checks" and "deprecation_checks" failed and there was no information why this failed.

As I understand from the discussion above, these checks will further fail until https://github.com/beutlich/ModelicaStandardLibrary/tree/update-binaries is moved to main (so any pull request that involves test/example models with tables will fail). Please, can you move these binaries to main, so that "external_c_checks" does no longer fail (I guess they need to be moved into Modelica/Resources/Library/ it seems best to always move these libraries to the expected location, when they are rebuild).

Note, it is planned to have a feature freeze today (12.01.2024, 16:00)

beutlich commented 6 months ago

I do not see that the checks failed: https://github.com/modelica/ModelicaStandardLibrary/actions/runs/7499083151/job/20415248489 and https://github.com/modelica/ModelicaStandardLibrary/actions/runs/7499083151/job/20415248013

The CI always builds all libraries as needed.

MartinOtter commented 6 months ago

I do not see that the checks failed: https://github.com/modelica/ModelicaStandardLibrary/actions/runs/7499083151/job/20415248489 and https://github.com/modelica/ModelicaStandardLibrary/actions/runs/7499083151/job/20415248013

The CI always builds all libraries as needed.

See: https://github.com/modelica/ModelicaStandardLibrary/pull/4129

grafik

and in "Details" I did not find more details and especially no link to the github actions.

I did not manage to get tables from master to run locally on my machine (I copied c/h files and lib files from https://github.com/beutlich/ModelicaStandardLibrary/tree/update-binaries but did not help. Could you just make master self-contained, so that the Resources directory is correct. I don't understand how a feature freeze can be done, if such an import feature is not included in the master branch)

beutlich commented 6 months ago

It says in https://github.com/modelica/ModelicaStandardLibrary/actions/runs/6865804143

The hosted runner: GitHub Actions 2 lost communication with the server.

and

The hosted runner: GitHub Actions 3 lost communication with the server.

which should not give any valid reason to revert a PR without proper review in a rush. CI should have just be triggered again.