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

Enforce module naming #828

Merged
merged 33 commits into from
Feb 22, 2023
Merged
Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
194f051
Add `--no-module-names` switch to fpm build settings
perazz Jan 12, 2023
239e28a
Improve help description
perazz Jan 12, 2023
d3509e9
Pass module naming flag to `fpm_model_t`
perazz Jan 12, 2023
d6d0515
Implement name enforcing rule function and basic unit test
perazz Jan 12, 2023
56a82f5
Add unit tests of invalid module names
perazz Jan 12, 2023
cdb9f54
Refactor: move `use`d to top of module
perazz Jan 12, 2023
107888e
Enable package names with dashes `-` (also test it)
perazz Jan 12, 2023
4c39860
Implement module name checking
perazz Jan 12, 2023
cab41da
Improve error output
perazz Jan 12, 2023
2163058
Implement new naming policy; update tests
perazz Jan 19, 2023
9633604
do not enforce module names by default; remove CLI option `--no-modul…
perazz Jan 19, 2023
39343e4
add `module-naming` option to the manifest under `[build]`
perazz Jan 19, 2023
04b78ad
add manifest tests for `module-naming` fpm.toml flag
perazz Jan 19, 2023
50e0901
`test_manifest.f90`: remove unused variables
perazz Jan 19, 2023
6b90e47
Ensure package name does not end with `_`
perazz Jan 20, 2023
1abf870
cleanup
perazz Jan 20, 2023
01141ce
fix: case-insensitive naming check
perazz Jan 31, 2023
1c39290
fix typo
perazz Jan 31, 2023
dbfbd58
case_sensitive typo
perazz Jan 31, 2023
8c15a42
Parse optional custom prefix
perazz Feb 9, 2023
236e566
bugfix tests with new naming option input
perazz Feb 9, 2023
1b9ba7b
move naming functions to `fpm_strings.f90`
perazz Feb 9, 2023
f6f309d
test custom prefixes
perazz Feb 9, 2023
f8a94f3
test standard and custom-prefixed module names
perazz Feb 9, 2023
bf4b837
query and check custom prefix from all dependencies
perazz Feb 9, 2023
396524c
add warning
perazz Feb 9, 2023
ee2bb68
rename all test modules to `fpm_test*`
perazz Feb 9, 2023
691c9cc
bugfix: rename `testsuite` to `fpm_testsuite` in test main file
perazz Feb 9, 2023
7780d7e
restore original test module names: `fpm_test*`->`test*`
perazz Feb 17, 2023
60b90b8
do not enforce naming on test modules
perazz Feb 17, 2023
2a26611
move to the source loop
perazz Feb 17, 2023
7076752
Merge branch 'main' into enforce_module_naming
perazz Feb 17, 2023
9518b62
Merge branch 'main' into enforce_module_naming
awvwgk Feb 22, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,5 @@ build/*
.vscode/

# CodeBlocks
project/*
project/

111 changes: 108 additions & 3 deletions src/fpm.f90
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
module fpm
use fpm_strings, only: string_t, operator(.in.), glob, join, string_cat, &
lower, str_ends_with
lower, str_ends_with, is_fortran_name, str_begins_with_str, &
is_valid_module_name, len_trim
use fpm_backend, only: build_package
use fpm_command_line, only: fpm_build_settings, fpm_new_settings, &
fpm_run_settings, fpm_install_settings, fpm_test_settings, &
Expand Down Expand Up @@ -92,6 +93,8 @@ subroutine build_model(model, settings, package, error)
model%build_prefix = join_path("build", basename(model%compiler%fc))

model%include_tests = settings%build_tests
model%enforce_module_names = package%build%module_naming
model%module_prefix = package%build%module_prefix

allocate(model%packages(model%deps%ndep))

Expand All @@ -107,7 +110,7 @@ subroutine build_model(model, settings, package, error)
model%packages(i)%name = dependency%name
call package%version%to_string(version)
model%packages(i)%version = version

if (allocated(dependency%preprocess)) then
do j = 1, size(dependency%preprocess)
if (dependency%preprocess(j)%name == "cpp") then
Expand Down Expand Up @@ -153,6 +156,11 @@ subroutine build_model(model, settings, package, error)
if (allocated(dependency%build%external_modules)) then
model%external_modules = [model%external_modules, dependency%build%external_modules]
end if

! Copy naming conventions from this dependency's manifest
model%packages(i)%enforce_module_names = dependency%build%module_naming
model%packages(i)%module_prefix = dependency%build%module_prefix

end associate
end do
if (allocated(error)) return
Expand Down Expand Up @@ -233,7 +241,11 @@ subroutine build_model(model, settings, package, error)
write(*,*)'<INFO> CXX COMPILER OPTIONS: ', model%cxx_compile_flags
write(*,*)'<INFO> LINKER OPTIONS: ', model%link_flags
write(*,*)'<INFO> INCLUDE DIRECTORIES: [', string_cat(model%include_dirs,','),']'
end if
end if

! Check for invalid module names
call check_module_names(model, error)
if (allocated(error)) return

! Check for duplicate modules
call check_modules_for_duplicates(model, duplicates_found)
Expand Down Expand Up @@ -286,6 +298,99 @@ subroutine check_modules_for_duplicates(model, duplicates_found)
end do
end subroutine check_modules_for_duplicates

! Check names of all modules in this package and its dependencies
subroutine check_module_names(model, error)
type(fpm_model_t), intent(in) :: model
type(error_t), allocatable, intent(out) :: error
integer :: i,j,k,l,m
logical :: valid,errors_found,enforce_this_file
type(string_t) :: package_name,module_name,package_prefix

errors_found = .false.

! Loop through modules provided by each source file of every package
! Add it to the array if it is not already there
! Otherwise print out warning about duplicates
do k=1,size(model%packages)

package_name = string_t(model%packages(k)%name)

! Custom prefix is taken from each dependency's manifest
if (model%packages(k)%enforce_module_names) then
package_prefix = model%packages(k)%module_prefix
else
package_prefix = string_t("")
end if

! Warn the user if some of the dependencies have loose naming
if (model%enforce_module_names .and. .not.model%packages(k)%enforce_module_names) then
write(stderr, *) "Warning: Dependency ",package_name%s // &
" does not enforce module naming, but project does. "
end if

do l=1,size(model%packages(k)%sources)

! Module naming is not enforced in test modules
enforce_this_file = model%enforce_module_names .and. &
model%packages(k)%sources(l)%unit_scope/=FPM_SCOPE_TEST

if (allocated(model%packages(k)%sources(l)%modules_provided)) then

do m=1,size(model%packages(k)%sources(l)%modules_provided)

module_name = model%packages(k)%sources(l)%modules_provided(m)

valid = is_valid_module_name(module_name, &
package_name, &
package_prefix, &
enforce_this_file)

if (.not.valid) then

if (enforce_this_file) then

if (len_trim(package_prefix)>0) then

write(stderr, *) "ERROR: Module ",module_name%s, &
" in ",model%packages(k)%sources(l)%file_name, &
" does not match its package name ("//package_name%s// &
") or custom prefix ("//package_prefix%s//")."
else

write(stderr, *) "ERROR: Module ",module_name%s, &
" in ",model%packages(k)%sources(l)%file_name, &
" does not match its package name ("//package_name%s//")."

endif

else

write(stderr, *) "ERROR: Module ",module_name%s, &
" in ",model%packages(k)%sources(l)%file_name, &
" has an invalid Fortran name. "

end if

errors_found = .true.

end if
end do
end if
end do
end do

if (errors_found) then

if (model%enforce_module_names) &
write(stderr, *) " Hint: Try disabling module naming in the manifest: [build] module-naming=false . "

call fatal_error(error,"The package contains invalid module names. "// &
"Naming conventions "//merge('are','not',model%enforce_module_names)// &
" being requested.")
end if

end subroutine check_module_names

subroutine cmd_build(settings)
type(fpm_build_settings), intent(in) :: settings
type(package_config_t) :: package
Expand Down
41 changes: 38 additions & 3 deletions src/fpm/manifest/build.f90
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,13 @@
!>```
module fpm_manifest_build
use fpm_error, only : error_t, syntax_error, fatal_error
use fpm_strings, only : string_t
use fpm_strings, only : string_t, len_trim, is_valid_module_prefix
use fpm_toml, only : toml_table, toml_key, toml_stat, get_value, get_list
implicit none
private

public :: build_config_t, new_build_config


!> Configuration data for build
type :: build_config_t

Expand All @@ -31,6 +30,10 @@ module fpm_manifest_build
!> Automatic discovery of tests
logical :: auto_tests

!> Enforcing of package module names
logical :: module_naming = .false.
type(string_t) :: module_prefix

!> Libraries to link against
type(string_t), allocatable :: link(:)

Expand Down Expand Up @@ -86,6 +89,35 @@ subroutine new_build_config(self, table, error)
return
end if

!> Module naming: fist, attempt boolean value first
call get_value(table, "module-naming", self%module_naming, .false., stat=stat)

if (stat == toml_stat%success) then

! Boolean value found. Set no custom prefix. This also falls back to
! key not provided
self%module_prefix = string_t("")

else

!> Value found, but not a boolean. Attempt to read a prefix string
call get_value(table, "module-naming", self%module_prefix%s)

if (.not.allocated(self%module_prefix%s)) then
call syntax_error(error,"Could not read value for 'module-naming' in fpm.toml, expecting logical or a string")
return
end if

if (.not.is_valid_module_prefix(self%module_prefix)) then
call syntax_error(error,"Invalid custom module name prefix for in fpm.toml: <"//self%module_prefix%s// &
">, expecting a valid alphanumeric string")
return
end if

! Set module naming to ON
self%module_naming = .true.

end if

call get_list(table, "link", self%link, error)
if (allocated(error)) return
Expand All @@ -95,7 +127,6 @@ subroutine new_build_config(self, table, error)

end subroutine new_build_config


!> Check local schema for allowed entries
subroutine check(table, error)

Expand All @@ -119,6 +150,9 @@ subroutine check(table, error)
case("auto-executables", "auto-examples", "auto-tests", "link", "external-modules")
continue

case ("module-naming")
continue

case default
call syntax_error(error, "Key "//list(ikey)%key//" is not allowed in [build]")
exit
Expand Down Expand Up @@ -156,6 +190,7 @@ subroutine info(self, unit, verbosity)
write(unit, fmt) " - auto-discovery (apps) ", merge("enabled ", "disabled", self%auto_executables)
write(unit, fmt) " - auto-discovery (examples) ", merge("enabled ", "disabled", self%auto_examples)
write(unit, fmt) " - auto-discovery (tests) ", merge("enabled ", "disabled", self%auto_tests)
write(unit, fmt) " - enforce module naming ", merge("enabled ", "disabled", self%module_naming)
if (allocated(self%link)) then
write(unit, fmt) " - link against"
do ilink = 1, size(self%link)
Expand Down
26 changes: 25 additions & 1 deletion src/fpm_model.f90
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ module fpm_model
use iso_fortran_env, only: int64
use fpm_compiler, only: compiler_t, archiver_t, debug
use fpm_dependency, only: dependency_tree_t
use fpm_strings, only: string_t, str
use fpm_strings, only: string_t, str, len_trim
implicit none

private
Expand Down Expand Up @@ -130,6 +130,10 @@ module fpm_model
!> Package version number.
character(:), allocatable :: version

!> Module naming conventions
logical :: enforce_module_names
type(string_t) :: module_prefix

end type package_t


Expand Down Expand Up @@ -179,6 +183,10 @@ module fpm_model
!> Whether tests should be added to the build list
logical :: include_tests = .true.

!> Whether module names should be prefixed with the package name
logical :: enforce_module_names = .false.
type(string_t) :: module_prefix

end type fpm_model_t

contains
Expand All @@ -199,6 +207,14 @@ function info_package(p) result(s)
if (i < size(p%sources)) s = s // ", "
end do
s = s // "]"

! Print module naming convention
s = s // ', enforce_module_names="' // merge('T','F',p%enforce_module_names) // '"'

! Print custom prefix
if (p%enforce_module_names .and. len_trim(p%module_prefix)>0) &
s = s // ', custom_prefix="' // p%module_prefix%s // '"'

s = s // ")"

end function info_package
Expand Down Expand Up @@ -343,6 +359,14 @@ function info_model(model) result(s)
! TODO: print `dependency_tree_t` properly, which should become part of the
! model, not imported from another file
s = s // ", deps=dependency_tree_t(...)"

! Print module naming convention
s = s // ', enforce_module_names="' // merge('T','F',model%enforce_module_names) // '"'

! Print custom prefix
if (model%enforce_module_names .and. len_trim(model%module_prefix)>0) &
s = s // ', custom_prefix="' // model%module_prefix%s // '"'

!end type fpm_model_t
s = s // ")"
end function info_model
Expand Down
Loading