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

should checkModule in masonPublish only count files? #25064

Closed
lucaferranti opened this issue May 16, 2024 · 5 comments
Closed

should checkModule in masonPublish only count files? #25064

lucaferranti opened this issue May 16, 2024 · 5 comments

Comments

@lucaferranti
Copy link
Contributor

lucaferranti commented May 16, 2024

Currently checkModule does

/* Checks to make sure the package only has one main module
 */
private proc moduleCheck(projectHome : string) throws {
  const subModules = listDir(projectHome + '/src');
  if subModules.size > 1 then return false;
  else return true;
}

hence listDir will count both files and directories, hence if my file has a structure

M.chpl
M/
    L.chpl

subModules will be 2, leading the test to fail, despite it has only one submodule and one main module. Currently the function checks that the src folder has only one child. Currently I think that test will only pass if src has exactly one file and no subdirectories.

Moreover, I am a bit puzzled by the soundness of the check, the idea of the function is to check that the function has only one main module. Shouldn't the correct logic be something like

private proc moduleCheck(projectHome : string) throws {
  const mainModules = listDir(projectHome + '/src', dirs=false); // count only files, ideally filter for only .chpl ones
  if mainModules.size == 1 then return true;
  else return false;
}

This would rely on the following assumptions

  1. files in src are considered main modules
  2. files in subfolders are submodules

which I believe is more in line with what described here and here.

on the other hand the documentation also says

For packages that span multiple files, the main module is designated by the module that shares the name with the package directory and the name field in the Mason.toml file.

so one could also use this logic in the check, although not sure if the compiler is aware of this.

@benharsh
Copy link
Member

I think you're absolutely right about the listDir being wrong. We'll take a look.

@lucaferranti
Copy link
Contributor Author

Thanks @benharsh for the reply! I think the fix itself (updating moduleCheck) will be relatively straightforward. I would like to understand the motivation for that check in the first place.

Suppose I have a library MyPkg. Here are three options for src/

Option 1

src/
   MyPkg.chpl

This will be fine, src has only one file, hence only one module, hence only one main module. However, this solution makes sense only for small and simple libraries.

Option 2

src/
   MyPkg.chpl
   something.chpl
   somethignElse.chpl

Do something.chpl and somethingElse.chpl count as submodules of MyPkg.chpl or are they peer main modules? If I read

or packages that span multiple files, the main module is designated by the module that shares the name with the package directory and the name field in the Mason.toml file.

I would understand from it that this structure should still be fine, since MyPkg.chpl is the only main module. However maybe this is a mason specific convention and can causes issues, so this could be forbidden I guess.

Option 3

src/
   MyPkg.chpl
   MyPkg/
       something.chpl
       somethingElse.chpl

This structure will currently fail (because of my diagnosis above) but based on my reading of the docs I think it should be accepted. Is there any problem with this? I tried a small toy example and mason build and mason test with no additional flags seemed to work for this.

@jeremiah-corrado
Copy link
Contributor

jeremiah-corrado commented May 20, 2024

I created a draft PR to fix this; however, it does allow Option 2 above, and I'm wondering whether that should be legal.

I suspect that the second case could be a problem if MyPkg.chpl has a public use for the modules something and/or somethingElse because there could be module-resolution conflicts if someone tries to use MyPkg in their project that already has a module with the same name. I'll investigate whether this is an issue before going further with my PR.

Or if anyone happens to already know that Option 2 should be illegal that would be helpful.

@jeremiah-corrado
Copy link
Contributor

Looking at this some more, I think we should make Option 2 illegal, to avoid potential module-resolution ambiguities. This requirement can always be relaxed in the future if we decide it's too strict.

jeremiah-corrado added a commit that referenced this issue May 21, 2024
…5084)

Fix the bug in Mason's main-module check, outlined in [this
issue](#25064), that
prevents packages that use sub-modules from being accepted in the mason
registry.

[reviewed by @bmcdonald3 ] - thanks!
@jeremiah-corrado
Copy link
Contributor

I think this issue can be closed due to #25084 and #25098

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants