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

WIP: Try harder to generate deps for Fortran module files #4452

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mwichmann
Copy link
Collaborator

@mwichmann mwichmann commented Dec 21, 2023

The Fortran Scanner finds INCLUDE and USE statements, and treats them the same - no dependency is generated if the indicated file does not exist. This is correct for include files, but not for module files - at least ones that are built as part of the project; if no dependency, the .mod file may not exist before it is used, and if so, SCons will abort with an error (like: Fatal Error: Cannot open module file 'module1.mod' for reading at (1): No such file or directory). With this change, module USE statements are treated differently: the dependency is created even if the module file does not yet exist.

Contributor Checklist:

  • I have created a new test or updated the unit tests to cover the new/changed functionality.
  • I have updated CHANGES.txt (and read the README.rst)
  • I have updated the appropriate documentation

SCons/Scanner/Fortran.py Outdated Show resolved Hide resolved
@mwichmann
Copy link
Collaborator Author

Change seems okay to me. I still want to know we have a test that actually tickles the problem in question - that is, fails before, works after.

@mwichmann
Copy link
Collaborator Author

And indeed - eliminating the incorrect USE statement for the MODULE PROCEDURE is worth doing in any case.

@mwichmann
Copy link
Collaborator Author

This has been updated a couple of times now to address some issues. Still needs to import the testcase that kicked all this off.

mwichmann added a commit to mwichmann/scons that referenced this pull request Jan 1, 2024
Tests that module inclusion generates the proper dependencies,
so that if the alphabetically-first file uses other modules,
thse module files are built first.

Signed-off-by: Mats Wichmann <mats@linux.com>
mwichmann added a commit to mwichmann/scons that referenced this pull request Jan 1, 2024
Tests that module inclusion generates the proper dependencies,
so that if the alphabetically-first file uses other modules,
thse module files are built first.

Fix to test framework to support file_fixture() taking a list
for dstfile.  The docstring says it does, but it didn't do the joining
that was done for srcfile.

Signed-off-by: Mats Wichmann <mats@linux.com>
@mwichmann
Copy link
Collaborator Author

Testcase reported in discussion #4451 is now incorporated.

@mwichmann
Copy link
Collaborator Author

mwichmann commented Jan 1, 2024

Recording an issue with this: originally, a mods_and_includes was computed, and cached in node.includes, so that if the same underlying file was requested for scanning again, the scan could be short-circuited and just use the cached value. After splitting these, there was no caching of the module files. Per the suggestion above, it was changed to cache the modules in node.attributes.modules. The surprise is that the attributes object is replaced and does not persist. This happens in the rfile method of the File class: if a variantdir is in use, which is the case for duplicate scanning anyway, it pulls the attributes object from the node representing the variant path and puts it into the node representing the original source path. This is PDB trace of that behavior:

> (trimmed-path)/SCons/Node/FS.py(3620)rfile()
-> result.attributes = self.attributes
(Pdb) p result.path, result.attributes, self.path, self.attributes
('src/b1.f', <SCons.Node.Node.Attrs object at 0x7f1a9ebdd050>, 'build/var3/b1.f', <SCons.Node.Node.Attrs object at 0x7f1a9eb9ab10>)
                                                                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
(Pdb) c
> (trimmed-path)/SCons/Node/FS.py(3620)rfile()
-> result.attributes = self.attributes
(Pdb) p result.path, result.attributes, self.path, self.attributes
('src/b1.f', <SCons.Node.Node.Attrs object at 0x7f1a9eb9ab10>, 'build/var4/b1.f', <SCons.Node.Node.Attrs object at 0x7f1a9eabc750>)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
(Pdb)

... see where the address of self.attributes - the first variant path scanned - becomes the address of the base source path result.attributes by the second time through, and then that will be replaced by a new Attrs object.

Comments at SCons.Node.FS.File.rfile describe the reasoning behind doing this, but as-is, it makes using attributes as a cache difficult.

So: how about if we went back to a combined modules-and-includes cache in node.includes, and on entry to the Fortran scanner routine, split that up if found, using (pseudo code):

suffix = env.subst('$FORTRANMODSUFFIX')
for file in mods_and_includes:
    if file.endswith(suffix):
        # put in modules
    else:
        # put in includes

@bdbaddog
Copy link
Contributor

bdbaddog commented Jan 3, 2024

Hmm.. so this line:
https://github.com/SCons/scons/blob/master/SCons/Node/FS.py#L3618
Is the line in question?
If so perhaps the solution is to store the modules in
node.rfile().attributes.modules instead of node.attributes.modules ?

And/or add a modules attributes to File() since we may(?) need such to handle c++ modules?

@mwichmann
Copy link
Collaborator Author

I guess we could do either way. Adding a modules might be useful - as you say, C++ may need it, and the D Interface File is basically a module file. Using rfile() might help. Or just go back to being combined in node.includes - since they are a form of include file, just handled differently, and splitting them on retrieval. I don't have any real opinion on what might be better (at least until trying them out).

@mwichmann
Copy link
Collaborator Author

Hmmm, not quite sure what the WIndows test failure is. I don't have this setup - it doesn't find gfortran in my case. This looks okay - no errors during build - but then the executable is not there. Notice the built objects use different commandline options than the executable, is it mismatching compiler/linker in this setup, and could that be the problem?

482/1279 (37.69%) C:\Python36\python.exe test\Fortran\USE-MODULE-live.py
C:\projects\scons\scripts\scons.py returned 2
STDOUT =========================================================================
scons: Reading SConscript files ...
scons: done reading SConscript files.
scons: Building targets ...
gfortran -o src\module1.obj -c -Jfortran_mods src\module1.f90
gfortran -o src\module2.obj -c -Jfortran_mods src\module2.f90
gfortran -o src\module0.obj -c -Jfortran_mods src\module0.f90
gfortran -o src\utils\util_module.obj -c -Jfortran_mods src\utils\util_module.f90
gfortran -o src\main.obj -c -Jfortran_mods src\main.f90
gfortran /OUT:main.exe src\main.obj src\module0.obj src\module1.obj src\module2.obj src\utils\util_module.obj
scons: building terminated because of errors.
STDERR =========================================================================
gfortran: error: /OUT:main.exe: No such file or directory
scons: *** [main.exe] Error 1

@bdbaddog
Copy link
Contributor

bdbaddog commented Jan 7, 2024

Hmmm, not quite sure what the WIndows test failure is. I don't have this setup - it doesn't find gfortran in my case. This looks okay - no errors during build - but then the executable is not there. Notice the built objects use different commandline options than the executable, is it mismatching compiler/linker in this setup, and could that be the problem?

482/1279 (37.69%) C:\Python36\python.exe test\Fortran\USE-MODULE-live.py
C:\projects\scons\scripts\scons.py returned 2
STDOUT =========================================================================
scons: Reading SConscript files ...
scons: done reading SConscript files.
scons: Building targets ...
gfortran -o src\module1.obj -c -Jfortran_mods src\module1.f90
gfortran -o src\module2.obj -c -Jfortran_mods src\module2.f90
gfortran -o src\module0.obj -c -Jfortran_mods src\module0.f90
gfortran -o src\utils\util_module.obj -c -Jfortran_mods src\utils\util_module.f90
gfortran -o src\main.obj -c -Jfortran_mods src\main.f90
gfortran /OUT:main.exe src\main.obj src\module0.obj src\module1.obj src\module2.obj src\utils\util_module.obj
scons: building terminated because of errors.
STDERR =========================================================================
gfortran: error: /OUT:main.exe: No such file or directory
scons: *** [main.exe] Error 1

Looks like it's using MSLINK syntax and LINK=gfortran...

@mwichmann
Copy link
Collaborator Author

Hmm, it seems I coded the testcase to use gfortran. So it's partly self-inflicted, but still want to explore.

@mwichmann
Copy link
Collaborator Author

After a couple of missteps, this looks better:

482/1279 (37.69%) C:\Python36\python.exe test\Fortran\USE-MODULE-live.py
PASSED
Test execution time: 2.6 seconds

@mwichmann
Copy link
Collaborator Author

mwichmann commented Jan 14, 2024

After even more digging, I now think this specific problem has to do with the failure to handle $FORTRANMODDIR correctly. If a module directory is on the command line, the compiler will implicitly include it as a place to search for module files. But the scan function in SCons/Scanner/Fortran doesn't know that, so it actually needs to add it to the path argument that's eventually passed on to find_file, or it won't look there. Enabling the debug in find_file:

scons: Building targets ...
  find_file: looking for 'module1.mod' in 'src' ...
  find_file: looking for 'module2.mod' in 'src' ...
gfortran -o src/module1.o -c -Jfortran_mods src/module1.f90
gfortran -o src/module2.o -c -Jfortran_mods src/module2.f90
gfortran -o src/module0.o -c -Jfortran_mods src/module0.f90
gfortran -o src/utils/util_module.o -c -Jfortran_mods src/utils/util_module.f90
  find_file: looking for 'module0.mod' in 'src' ...
  find_file: looking for 'util_module.mod' in 'src' ...

No hits. But add the module dir (fortran_mods) to the path passed by the scan function, and we get much better results, with no other code changes:

scons: Building targets ...
  find_file: looking for 'module1.mod' in 'src' ...
  find_file: looking for 'module1.mod' in 'fortran_mods' ...
  find_file: ... FOUND 'module1.mod' in 'fortran_mods'
  find_file: looking for 'module2.mod' in 'src' ...
  find_file: looking for 'module2.mod' in 'fortran_mods' ...
  find_file: ... FOUND 'module2.mod' in 'fortran_mods'
gfortran -o src/module1.o -c -Jfortran_mods src/module1.f90
gfortran -o src/module2.o -c -Jfortran_mods src/module2.f90
gfortran -o src/module0.o -c -Jfortran_mods src/module0.f90
gfortran -o src/utils/util_module.o -c -Jfortran_mods src/utils/util_module.f90
  find_file: looking for 'module0.mod' in 'src' ...
  find_file: looking for 'module0.mod' in 'fortran_mods' ...
  find_file: ... FOUND 'module0.mod' in 'fortran_mods'
  find_file: looking for 'util_module.mod' in 'src' ...
  find_file: looking for 'util_module.mod' in 'fortran_mods' ...
  find_file: ... FOUND 'util_module.mod' in 'fortran_mods'

Note: this scenario appears to apply to the D scanner as well. That's a non-breaking problem there (failure to find the .di file is just considered a loss of optimization, you fall back to the .d file), but it won't find the .di files in a directory given by $DI_FILE_DIR because the scanner doesn't pass that path along.

@mwichmann
Copy link
Collaborator Author

mwichmann commented Jan 15, 2024

Adding a variant_dir test to this breaks things again, so there's more to think about. The default mode, duplicate=True doesn't seem to work at all (some debug in the emitter added, and debug in the file finder turned on):

scons: Reading SConscript files ...
emitter looking for build/main.f90...rfile returns build/main.f90
Could not locate main.f90
emitter looking for build/module0.f90...rfile returns build/module0.f90
Could not locate module0.f90
emitter looking for build/module1.f90...rfile returns build/module1.f90
Could not locate module1.f90
emitter looking for build/module2.f90...rfile returns build/module2.f90
Could not locate module2.f90
emitter looking for build/utils/util_module.f90...rfile returns build/utils/util_module.f90
Could not locate util_module.f90
scons: done reading SConscript files.
scons: Building targets ...
gfortran -o build/main

The code in _fortranEmitter just gave up (this seems to be a test unique to Fortran):

    if not node.exists() and not node.is_derived():
        print("Could not locate " + str(node.name))
        return [], []

If you flip the duplicate argument to False, it works... differently:

scons: Reading SConscript files ...
emitter looking for build/main.f90...rfile returns src/main.f90
emitter looking for build/module0.f90...rfile returns src/module0.f90
emitter looking for build/module1.f90...rfile returns src/module1.f90
emitter looking for build/module2.f90...rfile returns src/module2.f90
emitter looking for build/utils/util_module.f90...rfile returns src/utils/util_module.f90
scons: done reading SConscript files.
scons: Building targets ...
looking for 'module1.mod' in 'src' ...
looking for 'module1.mod' in 'fortran_mods' ...
looking for 'module2.mod' in 'src' ...
looking for 'module2.mod' in 'fortran_mods' ...
gfortran -o build/module0.o -c -Jbuild/fortran_mods -Jsrc/fortran_mods src/module0.f90
gfortran -o build/module1.o -c -Jbuild/fortran_mods -Jsrc/fortran_mods src/module1.f90
gfortran -o build/module2.o -c -Jbuild/fortran_mods -Jsrc/fortran_mods src/module2.f90
gfortran -o build/utils/util_module.o -c -Jbuild/fortran_mods -Jsrc/fortran_mods src/utils/util_module.f90
looking for 'module0.mod' in 'src' ...
looking for 'module0.mod' in 'fortran_mods' ...
looking for 'util_module.mod' in 'src' ...
looking for 'util_module.mod' in 'fortran_mods' ...
gfortran -o build/main.o -c -Jbuild/fortran_mods -Jsrc/fortran_mods src/main.f90
gfortran -o build/main build/main.o build/module0.o build/module1.o build/module2.o build/utils/util_module.o

That's nominally correct-looking, but the gfortran command lines produced run directly into the issues that PR #4380 is aiming at - it's produced two -J arguments, which is illegal command-line syntax to gfortran:

f951: Fatal Error: gfortran: Only one '-J' option allowed

EDITED: actually the non-duplicating case isn't right either, there's still a path problem in it:

looking for 'module1.mod' in 'src' ...
looking for 'module1.mod' in 'fortran_mods' ...
looking for 'module2.mod' in 'src' ...
looking for 'module2.mod' in 'fortran_mods' ...

it actually needs to be looking in build/fortran_mods...

@mwichmann
Copy link
Collaborator Author

Attaching the test case for the above (rather than pushing as part of PR, yet), in case someone can spot my errors.

fort-variant.tar.gz

The Fortran Scanner finds INCLUDE and USE statements, and treats them the
same - no dependency is generated if the indicated file does not exist.
This is correct for include files, but not for module files - at least
ones that are built as part of the project; if no depdency, the .mod file
may not exist before it is used, and if so, scons will abort with an error
("Fatal Error: Cannot open module file 'module1.mod' for reading at (1):
No such file or directory"). With this change, module USE statements are
treated a bit differently. While they are still cached with includes,
the modules are split apart if the cache is found; and the FORTRANMODDIR
when using the file finder to search for files from the scanner.

Rquired fixing test Fortran syntax to use MODULE PROCEDURE in proper
context. Remove use MOD_BAR as you don't use a MODULE PROCEDURE via use
Added new test case: tests that module inclusion generates the proper
dependencies, so that if the alphabetically-first file uses other modules,
thse module files are built first.

Fix to test framework to support file_fixture() taking a list for dstfile.
The docstring says it does, but it didn't do the joining that was done
for srcfile.

Signed-off-by: Mats Wichmann <mats@linux.com>
@mwichmann mwichmann changed the title Unconditionally generate deps for Fortran module files Try harder to generate deps for Fortran module files Jan 18, 2024
…loads gfortran and gnulink, but doesn't explicitly specify the F90 and LINK variables. Let the tools do the right things. This also enables gfortran tool to load a differently named compiler (macports names them gfortran-mp-##)
@bdbaddog
Copy link
Contributor

Attaching the test case for the above (rather than pushing as part of PR, yet), in case someone can spot my errors.

fort-variant.tar.gz
Checked your example in here. https://github.com/bdbaddog/scons-bugswork/tree/main/fotran_modules/fort-variant
Added some alternative FORTRANMODDIRS and the ability to control duplicate via arg to SCons scons duplicate=1

@bdbaddog
Copy link
Contributor

I think the only thing we need before merging is a note in FORTRANMODDIR to explain that the path will always be relative to the build root unless it's passed as a Dir() node?

@mwichmann mwichmann changed the title Try harder to generate deps for Fortran module files WIP: Try harder to generate deps for Fortran module files Mar 14, 2024
@mwichmann
Copy link
Collaborator Author

marking WIP for the time being.

@mwichmann mwichmann added Fortran Fortran support issues scanner labels May 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fortran Fortran support issues scanner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants