chapel-lang / mason-registry

Package registry for mason, Chapel's package manager
17 stars 25 forks source link

"Adding ForwardModeAD@0.2.0 package to registry via mason publish" #74

Closed lucaferranti closed 5 months ago

lucaferranti commented 5 months ago

new release of ForwardModeAD to support chapel 2.0

lucaferranti commented 5 months ago

Before merging, please see my question in #62 . Do I need to manually create the tag release even if I ran mason publish?

benharsh commented 5 months ago

mason publish is supposed to add the tag, but there appears to be a bug here. I'll be opening an issue shortly to capture this problem, and will update this comment with a link.

In the meantime, I think the best thing would be to just create the tag manually.

We are also looking in to the cause of these CI failures and will get back to you when we know more!

lucaferranti commented 5 months ago

ok it seems we've unlocked the next level of of the mason publish quest

Main Module Check:
   Packages with more than one modules cannot be published. (FAILED)
------------------------------------------------------
Checking for fields in manifest file:
   Missing fields in manifest file (Mason.toml). (FAILED)
   The missing fields are as follows: 
   source

I guess the second one is just the Mason.toml having changed format for 2.0 and me failing to properly check the docs. I'll look at it.

The first one is a bit more interesting. I think of my library as a a single module, but I also want to structure that in multiple files for better readability/modularity. The main file (in this case ForwardModeAD.chpl) is just

module ForwardModeAD {

  public use Math;

  public use dualtype;

  public use arithmetic;

  public use trigonometric;

  public use transcendental;

  public use hyperbolic;

  public use differentiation;
}

The dualtype defines the dual number types. The files from arithmetic to hyperbolic overload functions for dual numbers and the last file has the logic to compute derivatives.

The main file only imports and re-exports all "sub-modules". It seems Mason does not like it anymore after 2.0? Why is that?

(feedback on how to better structure my library is of course also welcome, if my setup has some drawbacks. I do find having a lot of code in a single file a bit unergonomic though and prefer to split in multiple files. Especially for bigger libraries (although mine arguably is not there yet) being able to structure it in a modular way sounds pretty important to me).

lucaferranti commented 5 months ago

For the second one:

I checked the documentation for Manifest file (https://chapel-lang.org/docs/main/tools/mason/guide/manifestfile.html) and as far as I can tell, the source field seems to belong to the Mason.lock file but not to Mason.toml.

The toml file commited here (0.2.0.toml) does have the source field.

What is the idea of that check?

lucaferranti commented 5 months ago

Fixed the missing source field issue :tada: (and opened an issue about updating the documentation for it)

What about the other problem

(FAILED) Your package has more than one main module

?

I see the same issue in #73 , so it seems a common pattern (technically 2/3 of the packages trying to support Chapel 2.0 have this problem :P ), what is the motivation behind that restriction? Can it be lifted?

lucaferranti commented 5 months ago

ok, I think I've found something. My structure is flawn, as I should use a ForwardModeAD folder for the submodules. Nevertheless the package in #73 has a proper structure and the check still fails. I believe this is a bug in checkModule logic and I've opened an issue to discuss about it

lucaferranti commented 5 months ago

I see someone restarted the workflow of this a few minutes ago. I need to still fix the structure of my package, doing it now :)

Besides, wouldn't I need to wait for the next release anyway? Or does the registry use Chapel nightly build?

benharsh commented 5 months ago

That's my bad, I forgot that the package still needed updating. No need to rush and fix it right now!

If I'm reading things correctly then the CI job should be pulling from main, and therefore should be able to incorporate the "more than one module" fix from yesterday.

lucaferranti commented 5 months ago

If I'm reading things correctly then the CI job should be pulling from main, and therefore should be able to incorporate the "more than one module" fix from yesterday.

I see, interesting idea, why not just pulling the latest stable docker image? Is it because the registry is still "maturing" and it's more valuable to have latest main with potential fixes (like in this case)?

benharsh commented 5 months ago

Is it because the registry is still "maturing" and it's more valuable to have latest main with potential fixes (like in this case)?

I believe that's mainly it, yes.

lucaferranti commented 5 months ago

Ah, now I remember why I hadn't update the structure yet, I needed to ask something about modules includes and visibiliy, but I'll ask that on discourse, since it's probably something useful to other chapel library developers too

edit: funnily enough I had asked this question 1 year ago (for curiosity, before having the issue) https://chapel.discourse.group/t/importing-files-from-subfolders/17064, that's what I call thinking ahead of time xD

lucaferranti commented 5 months ago

posted a follow-up comment on discourse for better visibility: https://chapel.discourse.group/t/importing-files-from-subfolders/17064/8

here is the PR shuffling files around if someone wants to have a look: https://github.com/lucaferranti/ForwardModeAD/pull/16 feedback on what is an idiomatic structure welcome (except for your library is small, just put everything in one file :P)

The PR should in the end be fairly simple to navigate:

should hence be enough to look at the first 3 lines of each file, the rest is just some boring maths :)

lucaferranti commented 5 months ago

@benharsh CI green and sunny :sun_with_face:

benharsh commented 5 months ago

Thanks for sticking with it while we worked through these CI issues!