pharo-project / pharo

Pharo is a dynamic reflective pure object-oriented language supporting live programming inspired by Smalltalk.
http://pharo.org
Other
1.21k stars 355 forks source link

When filing in classes that have sub tags, a separate package is created. It should be within the parent package. #11211

Open StewMacLean opened 2 years ago

StewMacLean commented 2 years ago

When filing in classes that have sub tags, a separate package is created. It should be within the parent package.

Expected behavior The classes should be sub tagged within the parent package.

Version information:

Expected development cost Nothing - a fix is attached to file out parent package classes first, so that a parent is found on import and separate packages are not created.

RPackage-fileOut.st.zip

melkyades commented 4 months ago

Same problem here after migrating my code from pharo 9 to pharo 12

melkyades commented 4 months ago

fyi, just tried loading the same code in Pharo 11 and there everything works as supposed

jecisc commented 4 months ago

Hi @melkyades,

I did a lot of fixes on packages and package tags in Pharo 12.

Can you give me a way to reproduce the problem to see if I missed a place?

melkyades commented 4 months ago

Hi Cyril, thanks for your work. Loading BaselineOfPowerlang from https://github.com/powerlang/egg/ should be a reproducible case. I usually do it with a makefile so I haven't tried to do it manually though. Loading Portland core in pharo 11 generates the powerlang-scompiler package with a few tags. In pharo 12 it generates many packages instead.

GitHub
GitHub - powerlang/egg: Egg Smalltalk
Egg Smalltalk. Contribute to powerlang/egg development by creating an account on GitHub.
jecisc commented 4 months ago

Pharo 11 uses Tonel export format v1 that way exporting "categories". A category is a string representing the package name concatenated to the tag name. This is ambiguous and was causing a lot of troubles.

In Pharo 12 I introduced a new export format that still exports the category for backward compatibility, but it now exports the package and tag info separated.

I see here that your project uses the new format, but the infos exported are not right.

For example I'm seen this:

Class {
    #name : 'SCommentNode',
    #superclass : 'SParseNode',
    #instVars : [
        'value'
    ],
    #category : 'Powerlang-SCompiler-Parser',
    #package : 'Powerlang-SCompiler-Parser'
}

But this should be:

Class {
    #name : 'SCommentNode',
    #superclass : 'SParseNode',
    #instVars : [
        'value'
    ],
    #category : 'Powerlang-SCompiler-Parser',
    #package : 'Powerlang-SCompiler',
    #tag : 'Parser'
}

I don't know why this is wrong, maybe the code got exported from an image that had the categories problems. But is the packages and tags are fixed in Pharo 12 and that you keep committing from Pharo 12 or 13, things should be fine now.

melkyades commented 4 months ago

I see, maybe because of going forward and backward from previous versions to p12, something got borked.

Now, even if the format is ambiguous shouldn't it try to determine the packages from the dir structure? Because each package is a separate directory. Maybe pharo could write a log warning if it detects an inconsistency while loading a package

jecisc commented 4 months ago

It does it if the #package and #tag properties are not here. If they are here, there should be no ambiguity so there is no check

melkyades commented 4 months ago

Ok, but the #package doesn't match the dir, could that be checked?

And also (probably too late), having the dir named as the package, why do you need the #package key?

jecisc commented 4 months ago

This could be a check to add indeed.

We need the package because Tonel format can be used for single files. An project is to use it as a format for file out of a class and to be able to load a simple class like this. The folder structure comes only if you export a full project. So we need to have the information of the package in the Tonel class definition

melkyades commented 4 months ago

I manually fixed this for powerlang because I thought I had messed the repo somehow myself by going back and forth between p11 and p12. But now just checked in another repo, Webside project, and after loading I see the same kind of problems.

The repo defines only two packages: Webside and Webside-Tests. Let's look for example at WebsideServer class in Webside directory. The file says:

Class {

name : #WebsideServer,

#superclass : #Object,
#instVars : [ ... ],
#category : #'Webside-Base'

}

I think it is unambiguous from the file structure and class definition that WebsideServer is in Base tag inside Webside package, and not in Webside-Base package, but p12 puts it in Webside-Base package.

IMHO, that behavior should be reverted, as this is really problematic because it is code that was loading perfectly ok in p11 and now is incorrectly loaded in p12. If after loading it in p12 you open the iceberg repo you see the package as dirty, showing lots of classes as removed. There must be hundreds of repos elsewhere having the same kind problem.

btw, I'm not sure if this is the same than the original issue reported.

jecisc commented 4 months ago

I'll put on my todo list to check this issue because there should be a check to ensure classes are in the right package when we load a project in V1 tonel format in Pharo 12. This is surprising that the package is wrong.

melkyades commented 4 months ago

Yes, it was surprising to me too. Let me know if there's any other way in which I can help.

jecisc commented 1 month ago

I tried to load Powerlang-Core in Pharo 13 and I have only one package with the right tags