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

Restore ModelicaUtilities.h #4487

Draft
wants to merge 1 commit into
base: maint/4.1.x
Choose a base branch
from

Conversation

beutlich
Copy link
Member

@beutlich beutlich commented Oct 21, 2024

This reverts #3871 for main/4.1.x.

@beutlich beutlich force-pushed the restore-ModelicaUtilities.h branch from 39f3f5c to 8fceb4f Compare October 21, 2024 19:16
@beutlich beutlich added L: C-Sources Issue addresses Modelica/Resources/C-Sources L: Resources Issue addresses Modelica/Resources (excl. C-Sources) CI Issue that addresses continuous integration labels Oct 21, 2024
@@ -120,7 +120,7 @@ separated by LF or CR-LF.
pure function error "Print error message and cancel all actions - in case of an unrecoverable error"
extends Modelica.Icons.Function;
input String string "String to be printed to error message window";
external "C" ModelicaError(string) annotation(Include="#include \"ModelicaUtilities.h\"", Library="ModelicaExternalC");
Copy link
Contributor

@henrikt-ma henrikt-ma Oct 24, 2024

Choose a reason for hiding this comment

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

At the moment, it is a tool de-facto responsibility to ensure #include "ModelicaUtilities.h" will find the file, so doesn't this change belong in a separate discussion about how Modelica external functions should point to ModelicaUtilities.h?

@@ -1,4 +1,4 @@
#include "ModelicaUtilities.h"
#include "../../Modelica/Resources/C-Sources/ModelicaUtilities.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

(Attaching comment to one of several possible places.)

Considering that ModelicaUtilities.h has prototypes for functions that should be provided by the tool, and not for functions provided by the MSL, wouldn't a mode natural location be inside ModelicaServices?

Suggested change
#include "../../Modelica/Resources/C-Sources/ModelicaUtilities.h"
#include "../../ModelicaServices/Resources/Include/ModelicaUtilities.h"

(Also note the use of the default include directory for the library.)

Copy link
Contributor

@henrikt-ma henrikt-ma left a comment

Choose a reason for hiding this comment

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

If we are now to reintroduce the file in the MSL, I expect it to be taken as an indication of how other libraries should also deal with the need to find the file when building their external code.

One of the concerns behind the earlier decision to not include ModelicaUtilities.h with the MSL was that the file is tied to a version of the language specification. This isn't really a problem for the MSL, since the MSL is verbally tied to a language version:

It is fully compliant to <a href="https://specification.modelica.org/maint/3.6/MLS.html\">Modelica Specification version 3.6 and it has been tested with Modelica tools from different vendors.

but becomes a problem if other libraries that may not even have any dependency on the MSL start making copies of the file.

Is this something to consider for this PR, or a concern to address in future work?

@casella
Copy link
Contributor

casella commented Oct 24, 2024

I had a closer look at this PR, but it contains a lot more that what I proposed in #4484.

The requirement in #4484 is to have a .h file only containing the definitions of the functions found in Section 12.9.6, to be used by people writing external functions that need to call ModelicaUtilities functions. Nothing more than that. Just a courtesy so you don't have to copy-and-paste from the specification.

This file is not meant to help building MSL external function libraries. BTW, we agreed that we won't even add them to the repo starting from the next version, as it is the tool vendor's responsibility to provide them.

@casella
Copy link
Contributor

casella commented Oct 24, 2024

If we are now to reintroduce the file in the MSL, I expect it to be taken as an indication of how other libraries should also deal with the need to find the file when building their external code.

I think they can happily copy it into their code base, we could even write that in the header. That's what everybody's been doing so far, but using tool-specific files, which is not good if you want to build tool-agnostic libraries.

@casella
Copy link
Contributor

casella commented Oct 24, 2024

Note that the MSL is standard, but not tool agnostic, it is well understood that each tool vendor needs to adapt it to its tool (implementing ModelicaServices low-level functions, compiling the tables C code, etc.) for a number of good reasons. What is standard are the interfaces, but not the implementations.

Adapting the MSL to a specific tool is a burden of each tool vendor, that needs to do it once for its own tool, which is OK. What we want to avoid is to give other library developers the burden of developing N versions of their libraries, one for each tool. Of course they should be able to just release one that works everywhere. The MA-provided ModelicaUtilities.h file simply fulfills this purpose.

@casella
Copy link
Contributor

casella commented Oct 24, 2024

Considering that ModelicaUtilities.h has prototypes for functions that should be provided by the tool, and not for functions provided by the MSL, wouldn't a mode natural location be inside ModelicaServices?

This seems to me a good argument.

@beutlich
Copy link
Member Author

beutlich commented Oct 24, 2024

I had a closer look at this PR, but it contains a lot more that what I proposed in #4484.

Yes, that's why I called it restoration.

Actually, the qualifiers for noreturn or format are really useful. I would not agree on a plain ModelicaUtilities.h without them. Why would we give up on such valuable information by design?

@casella
Copy link
Contributor

casella commented Oct 24, 2024

I had a closer look at this PR, but it contains a lot more that what I proposed in #4484.

Yes, that's why I called it restoration.

Actually, the qualifiers for noreturn or format are really useful. I would not agree on a plain ModelicaUtilities.h without them.

Agreed. In that case, we should also update what we wrote in the MLS, otherwise that would be weird. In fact, the MLS could remain as it is, with a footnote like: these are conceptual definitions, please see the ModelicaConstants.h in ModelicaServices/Resources for more detail. Or something like that.

Do you agree?

@henrikt-ma
Copy link
Contributor

henrikt-ma commented Oct 25, 2024

Agreed. In that case, we should also update what we wrote in the MLS, otherwise that would be weird. In fact, the MLS could remain as it is, with a footnote like: these are conceptual definitions, please see the ModelicaConstants.h in ModelicaServices/Resources for more detail. Or something like that.

Isn't it standard procedure in the C/C++ world to have this sort of difference between how the prototypes are presented in documentation and what they actually look like in a header from the real world?

On the same theme, wouldn't it be just ugly if the Modelica specification had to say exactly which macro name (MODELICA_UTILITIES_H) that is used for the include guard?

That said, I think we should also do something about the current presentation of ModelicaUtilities.h in the specification. At the moment, it looks like this:

This section describes the utility functions declared in ModelicaUtilities.h, which can be called in external Modelica functions written in C.

We could imagine something like this instead (to be made more concrete in a PR on the specification):

This section describes utility functions which can be called in external Modelica functions written in C. The functions are declared in modelica:/ModelicaServices/Resources/Include/ModelicaUtilities.h, which is written to work with a broad range of tools. The author of a Modelica library with build projects for its external code is responsible for making sure that the ModelicaUtilities.h used when compiling the external code corresponds to the correct version of the Modelica Language Specification.

@henrikt-ma
Copy link
Contributor

Not so sure about my own suggestion with Modelica URIs here… Would we really want to encourage a solution where the ugly practice of implementations contained in Include-annotations should have the following annotation in order to find ModelicaUtilities.h?

IncludeDirectory="modelica://ModelicaServices/Resources/Include"

It both seems inconvenient for the library author, and means that tools that – for reasons I can't really understand – would like to use their own ModelicaUtilities.h would need overwrite the file inside ModelicaServices.

If we had standardized the use of CMake for external code build projects, I would have proposed to explore the idea of relying on MODELICA_PATH for locating the correct directory to put in the include path. Imagine an MA-provided CMake function to be called with a MSL (not MLS) version:

include_directory_for_ModelicaServices("4.1.0");

Since most Modelica libraries are made to work with the MSL anyways, it doesn't seem like a too strong assumption that their build projects would be ready to point to a specific version of the MSL for finding ModelicaUtilities.h. A side effect of this is that since the MSL is made for a specific version of the MLS, this way of locating ModelicaUtilities.h is indirectly locating the ModelicaUtilities.h for that specific version of the MLS.

Based on this, I would prefer to not start making general use of Modelica URIs for locating ModelicaUtilities.h or its parent directory. Something specific like this could serve us better:

Newer versions of the Modelica Standard Library include a ModelicaUtilities.h corresponding to the used version of the Modelica Language Specification, located at modelica:/ModelicaServices/Resources/Include/ModelicaUtilities.h. In principle, when combined with the MODELICA_PATH, this allows external code build projects to determine an include directory for ModelicaUtilities.h based on a choice of version for the Modelica library.
[Milage may vary with how MODELICA_PATH is handled.]

Revert "refs modelica#3867: Remove LICENSE_ModelicaUtilities.txt"

This reverts commit 7fe506f.

Revert "refs modelica#3867: Remove ModelicaUtilities.h"

This reverts commit 1bb1d40.
@beutlich beutlich force-pushed the restore-ModelicaUtilities.h branch from 8fceb4f to 272e4d9 Compare December 28, 2024 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Issue that addresses continuous integration L: C-Sources Issue addresses Modelica/Resources/C-Sources L: Resources Issue addresses Modelica/Resources (excl. C-Sources)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants