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

Revert "Fix position prescribed friction" #4255

Merged
merged 1 commit into from Jan 12, 2024

Conversation

MartinOtter
Copy link
Member

@MartinOtter MartinOtter commented Jan 12, 2024

Reverts #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.

@MartinOtter MartinOtter merged commit ee5fae8 into master Jan 12, 2024
1 of 2 checks passed
@MartinOtter MartinOtter deleted the revert-4129-fix-friction branch January 12, 2024 07:15
@tobolar
Copy link
Contributor

tobolar commented Jan 12, 2024

Duplicate to #3911

@tobolar
Copy link
Contributor

tobolar commented Jan 12, 2024

@beutlich Is it documented how to proceed in such cases or where to find up-to-date binaries? #4178 (comment) is well and good but not that easy to find it.

@beutlich beutlich added L: Mechanics.Rotational Issue addresses Modelica.Mechanics.Rotational L: Mechanics.Translational Issue addresses Modelica.Mechanics.Translational V: 4.1.0-dev Issue originates in MSL v4.1.0-dev (and is not present in earlier releases) labels Jan 12, 2024
@beutlich beutlich added this to the MSL4.1.0 milestone Jan 12, 2024
@beutlich
Copy link
Member

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
Copy link
Member

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

@tobolar
Copy link
Contributor

tobolar commented Jan 12, 2024

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
Copy link
Member

beutlich commented Jan 12, 2024

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
Copy link
Member Author

MartinOtter commented Jan 12, 2024

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
Copy link
Member

m-kormann added a commit to m-kormann/ModelicaStandardLibrary that referenced this pull request Jan 12, 2024
…x-friction"

This reverts commit ee5fae8, reversing
changes made to ae7afdc.
@MartinOtter
Copy link
Member Author

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: #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)

MartinOtter added a commit that referenced this pull request Jan 12, 2024
Revert "Merge pull request #4255 from modelica/revert-4129-fix-friction"
@beutlich
Copy link
Member

beutlich commented Jan 12, 2024

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.

@beutlich beutlich mentioned this pull request Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: Mechanics.Rotational Issue addresses Modelica.Mechanics.Rotational L: Mechanics.Translational Issue addresses Modelica.Mechanics.Translational V: 4.1.0-dev Issue originates in MSL v4.1.0-dev (and is not present in earlier releases)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants