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

Immutable GAPInfo.PackagesInfo records: Problems for JuliaInterface and float #2568

Closed
fingolfin opened this issue Jun 20, 2018 · 6 comments
Closed
Labels
kind: bug Issues describing general bugs, and PRs fixing them regression A bug that only occurs in the branch, not in a release topic: packages issues or PRs related to package handling, or specific to a package (for packages w/o issue tracker)
Milestone

Comments

@fingolfin
Copy link
Member

fingolfin commented Jun 20, 2018

When I merged hpcgap/lib/package.gi into lib/package.gi, see 42789e6, I accidentally broke the float package, which modifies its package record after it has been set. It does that to modify its banner string, to indicate which parts of it where actually compiled and loaded. That is indeed quite useful information, and it makes sense to display that as part of the banner string.

One way to fix this is to stop making the package records immutable, at least in GAP. But they'll stay immutable in HPC-GAP, so float would be broken there, unless we can stop making the package record immutable there, too... or perhaps we can delay doing so until the package was fully loaded (i.e. both init.g and read.g have been read)?

So my preferred solution would be to work together with @laurentbartholdi to change float to not modify the package record during a late stage anymore. But right now, I have not yet made my mind up on what I think would be a good solution; also, perhaps changing the immutability of the package info record at a later stage can be implemented, then no change to float would be necessary.

This must be resolved before GAP 4.10.

@fingolfin fingolfin added kind: bug Issues describing general bugs, and PRs fixing them regression A bug that only occurs in the branch, not in a release topic: packages issues or PRs related to package handling, or specific to a package (for packages w/o issue tracker) labels Jun 20, 2018
@fingolfin fingolfin added this to the GAP 4.10.0 milestone Jun 20, 2018
@olexandr-konovalov
Copy link
Member

Why can't we make them atomic records in HPC-GAP instead of being immutable?

@fingolfin
Copy link
Member Author

Well, then the content of the records (i.e. strings, lists, other records) still would not be accessible from all threads. Of course that may not be a major problem in the end -- I did not extensively test this, I just copied changes that others applied there in the past (e.g. @ChrisJefferson it seems, based on my reading of git log, which may flawed, though).

@olexandr-konovalov
Copy link
Member

If you do atomic list of immutable things, then Float can simply replace one immutable banner by another immutable banner.

Or, maybe Float should simply change the way it modifies its record? Make a copy, modify it, make it immutable and replace the old record. Needs some experimenting.

@laurentbartholdi
Copy link
Contributor

laurentbartholdi commented Jun 20, 2018 via email

@stevelinton stevelinton changed the title Loading float fails in master Immutable GAPInfo.PackagesInfo records: Problems for JuliaInterface and float Jun 22, 2018
fingolfin added a commit to fingolfin/gap that referenced this issue Jul 2, 2018
This fixes loading of float and JuliaInterface in master, as described
in issue gap-system#2568, at least im regular GAP. For HPC-GAP, things remain
unchanged for now.
@fingolfin
Copy link
Member Author

While I like the idea of allowing a banner string function, setting it as the value for BannerString then also means that package won't work with older versions of GAP, so it's not really backwards compatible. But we could instead add a new field BannerFunction. Then old GAP versions will just keep printing the (default) BannerString, while new GAP versions will use the result of BannerFunction.

As an alternative (which is not mutually exclusive, i.e., we could do both), one could offer a way to add additional information into the output of DefaultPackageBannerString, to be printed at the end of that: Say a new field BannerExtras, which can be either a string or a zero-arg function returning a string; and that string is inserted into the default banner, right before the trailing separator.

@fingolfin
Copy link
Member Author

Or perhaps it should be functions taking one argument, namely the info record?

fingolfin added a commit that referenced this issue Jul 3, 2018
This fixes loading of float and JuliaInterface in master, as described
in issue #2568, at least im regular GAP. For HPC-GAP, things remain
unchanged for now.
rbehrends added a commit to rbehrends/gap that referenced this issue Apr 21, 2020
In order to make PackageInfo records accessible from other threads,
they need to be immutable, readonly, or atomic. Previously, they
were made immutable, but that broke packages that altered them after
initialization. The new workaround makes the package record itself
atomic and its contents immutable.

See issue gap-system#2568 for further discussion.
rbehrends added a commit to rbehrends/gap that referenced this issue Apr 21, 2020
In order to make PackageInfo records accessible from other threads,
they need to be immutable, readonly, or atomic. Previously, they
were made immutable, but that broke packages that altered them after
initialization. The new workaround makes the package record itself
atomic and its contents immutable.

See issue gap-system#2568 for further discussion.
rbehrends added a commit to rbehrends/gap that referenced this issue Oct 13, 2020
In order to make PackageInfo records accessible from other threads,
they need to be immutable, readonly, or atomic. Previously, they
were made immutable, but that broke packages that altered them after
initialization. The new workaround makes the package record itself
atomic and its contents immutable.

See issue gap-system#2568 for further discussion.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Issues describing general bugs, and PRs fixing them regression A bug that only occurs in the branch, not in a release topic: packages issues or PRs related to package handling, or specific to a package (for packages w/o issue tracker)
Projects
None yet
Development

No branches or pull requests

3 participants