Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add support for namespaced modules #823

Closed
wants to merge 6 commits into from

Conversation

perazz
Copy link
Member

@perazz perazz commented Jan 5, 2023

The STF project aims to enable support for namespaced modules (See #153, #819) as a means to prevent name collisions among modules of different packages.

This will require pretty substantial changes to fpm, so I'm opening a draft pull request to enable discussion and community review of this work.

Compiler/standard limitations will still limit the flexibility of this approach (e.g., directly useing modules with the same name from two different packages), but I don't think it is a good idea to have fpm modify any of the source codes on-the-fly. Anyways we can remove most of the friction to the user with how we design this feature. This is a tentative to-do list:

  • Define namespacing conventions for prefixing modules (default namespace, separator)
  • Extend module handling from a type(string_t) to a type(module_t) which contains all necessary information and procedures, with fallback to default behavior (aka no namespace) where no specifications are made
  • Automatic generation of namespaced module interfaces, e.g. with the package name as a prefix:
module namespace_FPM_modulename
   ! Import all public entities from the non-namespaced module
   use modulename, only: sub1, sub2, sub3
   use modulename, only: fun1, fun2
   use modulename, only: var1, var2

   implicit none(type,external)
   private

   public :: sub1, sub2, sub3
   public :: fun1, fun2
   public :: var1, var2
end module namespace_FPM_modulename
  • Add source files for the namespaced modules to the project (by default, or only when they're being used?) and to the build

As long as the object files of the non-namespaced modules stay within their own library file, there should be no issues even in presence of collisions, but FPM must prompt for them during build.

  • In presence of module naming collisions, prompt the package developer with humanly-readable error messages that tell them where non-namespaced modules should be replaced with namespaced ones, e.g.:
>  use module, only: sub1 
       ^^^^^^

a module naming collision was found at line XYZ in module ZYX.f90. 
The same module name was found in the following dependencies: 
- current project (/path/to/file/name)
- dependency1 (/path/to/file/name)
- ...

Please, modify the source code to employ a unique namespaced module name, as provided by fpm, e.g.: 
> use package_FPM_module, only: sub1

CC: @fortran-lang/admins @fortran-lang/fpm @certik @awvwgk @milancurcic @everythingfunctional @arteevraina @henilp105 @minhqdao

When the project name changes in fpm.toml, command `fpm update` is not able to update the dependency tree because of the different name of the root dependency. Allow `update` to instead build the whole project from scratch when its name has changed
@everythingfunctional
Copy link
Member

Just to make sure I understand the idea here. fpm will automatically generate a "namespaced" version of every module in every dependency a project uses? What about projects that are already following a "best practice" of prefixing their modules? Also, won't this still leave open the possibility for link-time name clashes? I.e.

module projA_FPM_mod
   ! Import all public entities from the non-namespaced module
   use mod, only: foo

   implicit none(type,external)
   private

   public :: foo
end module
module projB_FPM_mod
   ! Import all public entities from the non-namespaced module
   use mod, only: foo

   implicit none(type,external)
   private

   public :: foo
end module

There will still be 2 __mod_foo symbols present at link time.

@perazz
Copy link
Member Author

perazz commented Jan 5, 2023

Very smart comment, thank you. I want to set up some unit tests for these edge cases cause I want to see how compilers behave (is the global function name renamed/duplicated in the object table or not?).

One possible workaround to force each name to be in the namespaces module would be to copy the whole dummy argument list and also create wrappers, this should work at least for functions and subroutines, and parameter variables, but certainly not with non-parameter variables and classes:

module projB_FPM_mod
   ! Import all public entities from the non-namespaced module
   use mod, only: foo_orig => foo

   implicit none(type,external)
   private

   public :: foo

   contains 

   subroutine foo(same,dummy,args)
      call foo_orig(same,dummy,args)
   end subroutine foo

end module

But as said, I need to lay out some tests to define the solution that works for the most cases

@certik
Copy link
Member

certik commented Jan 6, 2023

I don't understand.

All we need is to enforce prefixing all module names with a prefix, that by default is the package name, and can be changed in fpm.toml. It's a simple check. Why wouldn't this work?

This PR seems very complicated.

@perazz
Copy link
Member Author

perazz commented Jan 6, 2023

Thank you @certik. I may have gotten too far with the design trying to account for all possible edge cases. You mean to rename all module names in all source files instead of creating prefixed module wrappers?

@certik
Copy link
Member

certik commented Jan 6, 2023

I think fpm should not rename anything, it should simply enforce the convention.

After that is implemented, we can brainstorm if anything else is needed, but probably not.

@perazz
Copy link
Member Author

perazz commented Jan 6, 2023

Sounds good @certik. The reason I had thought of adding the prefixed modules was to not break existing buids. I will rework this PR in the way you're suggesting and let's see what it looks like afterwards.

Is it OK that the prefix looks for an _FPM_ separator? Otherwise it could just be an underscore. Same thing, I would rename all special characters in the package name to underscore.

@everythingfunctional
Copy link
Member

Is it OK that the prefix looks for an _FPM_ separator?

I don't think there's any need for a separator. So long as a module name starts with the name of the package (or specified prefix), it's fine.

@perazz
Copy link
Member Author

perazz commented Jan 6, 2023

Yes it makes sense, and I also think this does not need a derived type for modules anymore. So I think I will open a new PR to start from a clean branch.

@certik
Copy link
Member

certik commented Jan 6, 2023

Yes, keep this PR open for reference, but open a new PR with a minimal clean implementation. Provide a command line option to disable this check, that way existing packages will still build, just not by default.

@everythingfunctional should we require underscore, that is: package name + "_" ?

@everythingfunctional
Copy link
Member

should we require underscore, that is: package name + "_" ?

I was going to go with no, as it simplifies the rules, even if one wants to have a single "public" module for a package. I.e. there is only module for strff, but it's named strff, which starts with the package name, so it satisfies the rule. No special cases needed. Most people probably will use an underscore for separation, but that's fine.

@certik
Copy link
Member

certik commented Jan 6, 2023

So probably also the fpm.toml optional custom prefix would have to contain _ explicitly, if that was to be used. I think that's fine.

@everythingfunctional
Copy link
Member

the fpm.toml optional custom prefix would have to contain _ explicitly

Why?

If the rule is "all of a package's library modules (i.e. those in src) must begin with the prefix, which defaults to the package name," module prefix_utils still satisfies the rule even if only prefix is what is specified.

@certik
Copy link
Member

certik commented Jan 7, 2023

If the rule is "all of a package's library modules (i.e. those in src) must begin with the prefix, which defaults to the package name," module prefix_utils still satisfies the rule even if only prefix is what is specified.

Good point, haven't thought of that. Although some authors might prefer the _ to be enforced too.

@perazz perazz mentioned this pull request Jan 12, 2023
@perazz
Copy link
Member Author

perazz commented May 31, 2023

This was superseded by #828 , I will close this PR now.

@perazz perazz closed this May 31, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants