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

KernelLibrary class>>#’isWine’ doesn’t work properly in runtime #255

Closed
wants to merge 2 commits into from

Conversation

jgfoster
Copy link

It checks to see if NTLibrary is present, but by default it is stripped. This causes it to be included. An alternate edit would be to change the KernelLibrary method. Comments and suggestions welcome!

…s NTLibrary is present. This causes it to be included.
Copy link
Contributor

@blairmcg blairmcg left a comment

Choose a reason for hiding this comment

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

This doesn't seem desirable - it's adding a package into the base pre-reqs of the image stripper package which should never depend on anything other than 'Dolphin'. This is only required because KernelLibrary>>isWine is using a symbolic binding to a class that it should directly reference, which it can't do because that would create a circular dependency from the base Dolphin package, which by definition cannot have prerequisites. I think it would be preferable to just move NTLibrary to the Dolphin package and then fix KernelLibrary>>isWine to use a normal class reference (or get rid of that method altogether and directly use NTLibrary>>isWine from all current call sites)

@jgfoster
Copy link
Author

Sounds good. I'll need to research how to put something into the Dolphin package. As I recall, at the beginning it was more complicated to modify. (In fact, everything in 'Dolphin Base' really should be in Dolphin but we didn't have a way to do that at first.)

@blairmcg
Copy link
Contributor

When I suggested that, I thought it would "just work". Adding stuff to the base is supposed to be easy since I modified the boot process to reload the system package before proceeding with package installation, however it looks like it is easy to add new methods/state to existing classes, but it doesn't actually load newly defined classes :-(. Let me look into that, as it is supposed to just work so you shouldn't have to invest a lot of effort.

@blairmcg
Copy link
Contributor

That's done. I moved NTLibrary to the Dolphin package to test it, so you can abandon this one if you like. With the change it is easy to add new BCL classes, although in general we should be looking to move stuff out, rather than adding more. I think the command line processing stuff justifies its own package, and I can't see a good reason to consolidate it into the BCL since there are no BCL dependencies on it. 'Dolphin Base' could be renamed/repurposed for that, since it now contains only the command line processing classes.

@jgfoster
Copy link
Author

Thanks!

@jgfoster jgfoster closed this Nov 16, 2016
@jgfoster jgfoster deleted the NTLibrary branch November 16, 2016 11:25
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

2 participants