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.

Sign up for GitHub

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

Jump to bottom

SCons does not handle fortran submodules and type bound procedures correctly. #3135

Closed
pdiener opened this issue May 31, 2018 · 28 comments
Closed

SCons does not handle fortran submodules and type bound procedures correctly. #3135

pdiener opened this issue May 31, 2018 · 28 comments

Comments

@pdiener
Copy link
Contributor

pdiener commented May 31, 2018

SCons seems to think that the declaration of the interface of type bound procedures in a module declares a module named function. This happens when the definition of the procedures are in submodules.

  • Version of SCons
    I have tried both SCons 3.0.1 and the latest development version SCons 3.1.0.alpha.

  • Version of Python
    Python 2.7.12

  • Which python distribution if applicable (python.org, cygwin, anaconda, macports, brew,etc)
    OpenSuse

  • How you installed SCons
    For version SCons 3.0.1:
    Downloaded and unpacked the tar ball from SourceForge cd'ed into the scons-3.0.1 directory and did 'python setup.py install' as root.
    For Version SCons 3.1.0.alpha:
    Cloned the git repository (git clone https://github.com/SCons/scons.git) cd'ed into the scons directory and did 'python bootstrap.py', 'cd build/scons' and finally 'python setup.py install' as root.

  • What Platform are you on? (Linux/Windows and which version)
    Linux OpenSuse 42.1

  • How to reproduce your issue? Please include a small self contained reproducer. Likely a SConstruct should do for most issues.
    In the following zip-file, there are 3 fortran source files and an SConstruct file that shows the issue.
    scons-submodule.zip
    When I run scons I get:
    scons: *** Multiple ways to build the same target were specified for: function.mod (from ['test_1.f90'] and from ['test_2.f90'])
    File "/home/diener/TMP/SCons_Test/SConstruct", line 28, in <module>

  • Link to SCons Users thread discussing your issue.
    https://pairlist4.pair.net/pipermail/scons-users/2018-April/006893.html
@pdiener
Copy link
Contributor Author

pdiener commented May 31, 2018

This is the link to the User thread.

@bdbaddog
Copy link
Contributor

The Fortran scanner needs to ignore this:

interface

  module subroutine set_n ( this, n )
    class(test_type_1), intent(inout) :: this
    integer, intent(in) :: n
  end subroutine

  module function get_n ( this )
    class(test_type_1), intent(in) :: this
    integer :: get_n
  end function get_n

end interface

The scanner seems to be processing the module function and expecting a function.mod to be generated.

@pdiener
Copy link
Contributor Author

pdiener commented Jun 15, 2018

I looked at the fortran scanner and found the regex that it uses to find module definitions (in src/engine/SCons/Scanner/Fortran.py). The regex is:

def_regex = """(?i)^\s*MODULE\s+(?!PROCEDURE)(\w+)"""

and so is designed to not match MODULE followed by the keyword PROCEDURE. I had the idea that I could get the scanner to correctly ignore the problematic module subroutine and module function by modifying the regex to:

def_regex = """(?i)^\s*MODULE\s+(?!PROCEDURE|SUBROUTINE|FUNCTION)(\w+)"""

as a valid module name should not be a fortran keyword anyway. However SCons still fails for my example after this change.

In order to ensure that the scanner had been updated in my build, I first changed my example to comment out any reference to the test_2 module, in which case the build is successful. I then added a print statement to the Fortran scanner to print out the value of the defmodules variable after the regex was used. Running this with the original regex, I could tell that the regex matched module test_1, module subroutine and module function as expected while, with the modified regex, it only matched module test_1. So the new regex does seem to work. After turning the test_2 module back on, SCons fails before executing the print statement from the Fortran Scanner.

It therefore seems to me that there is something else in SCons that triggers the error even before the Fortran scanner is called. Any ideas, where this might be?

@bdbaddog
Copy link
Contributor

@pdiener Can you fork the repo, branch, and push a pull request with your work in progress?

Then we can take a look at your work so far and help to resolve any remaining issues.

( See: https://github.com/SCons/scons/wiki/GitWorkflows )

@pdiener
Copy link
Contributor Author

pdiener commented Jun 18, 2018

Done. I hope I followed the instructions correctly.

@pdiener pdiener mentioned this issue Mar 4, 2019
Closed
3 tasks
@pdiener
Copy link
Contributor Author

pdiener commented Mar 4, 2019

I finally got time to take another look at this and I figured out that the same regex was used in the emitter in FortranCommon.py. Fixing the regex there allowed me to compile my project with scons. As a lot of time had passed, I deleted my fork of scons and created a new one. I have created a pull request based on a checkout of scons from today.

@jglezplatas jglezplatas mentioned this issue Apr 22, 2019
Closed
@jglezplatas
Copy link

Upss!
I found another problem with definitions on submodules in fortran.

Using the previous example I have modified a little and I added two additional fortran files (test_21.f90 and test_22.f90). Both should be ok in fortran sense
Scons with the modifications commented sometime ago works using test_1.f90 + test_2.f90 + test_21.f90+test_submodules.f90 but if test_21.f90 is replaced by test_22.f90 then scons again fail.
The main difference is that in test_22.f90 the subroutine again declare the arguments.

Javier

test_submodules.zip

@bdbaddog
Copy link
Contributor

@jglezplatas - Can you add a small snippet of fortran code to your comment showing this additional syntax you have in your zip file?

@jglezplatas
Copy link

The left picture is the file test_21.f90. The module procedure takes the information given in the interface block as you can see on the message of bdbaddog (31/may/2018) and scons works for this case.
While the right picture is the file test_22.f90 that give the problem with scons in the current version.
In this case the definition of the procedure (subroutine in this case) is given again (equal that in the interface block). Word "pure" is irrelevant in this case (or I think it).

submodules_fortran

@pdiener
Copy link
Contributor Author

pdiener commented Apr 22, 2019

I tried this example out and found that it is indeed the pure attribute does mess scons up so that it thinks there are multiple definitions of a module named pure. I suspect a simple fix is to edit the regexes to also ignore 'module' followed by 'pure'. I can't think of any case where those two keywords would follow each other, except for this case. Unless the programmer really intends to declare a module named 'pure', which would be incredibly stupid. I haven't tried the fix out though. While we're at it we probably also need to include 'elemental' in the regex as this is another keyword that can be used in front of 'subroutine' of 'subroutine'. And then of course there is the Fortran 2008 keyword
'impure' that can be used in front of an elemental procedure.

@bdbaddog
Copy link
Contributor

The left picture is the file test_21.f90. The module procedure takes the information given in the interface block as you can see on the message of bdbaddog (31/may/2018) and scons works for this case.
While the right picture is the file test_22.f90 that give the problem with scons in the current version.
In this case the definition of the procedure (subroutine in this case) is given again (equal that in the interface block). Word "pure" is irrelevant in this case (or I think it).

submodules_fortran

Pictures are not the preferred way to share code in here.
Please just paste the text and then use the right markup to mark it as code. (surround with triple single back quotes).

@jglezplatas
Copy link

jglezplatas commented Apr 22, 2019 via email

bdbaddog added a commit to bdbaddog/scons that referenced this issue Apr 25, 2019
@bdbaddog
Fix Issue SCons#3135 - Also add tests to check that emitter is proper…
e820fbf 
…ly processing interface module declarations
@bdbaddog bdbaddog mentioned this issue Apr 25, 2019
Merged
3 tasks
@bdbaddog
Copy link
Contributor

bdbaddog commented Apr 25, 2019 via email

@pdiener
Copy link
Contributor Author

pdiener commented Apr 25, 2019

In order to solve the issue with declaring a type bound procedure as being pure in the module and then repeating that declaration in the submodule, you would have to add PURE to the regex in both the scanner and emitter. I personally prefer to not have to repeat the declaration in the submodule (i.e. by using the "module procedure' syntax instead), but it is not illegal to do so, so it will probably be best
to expand the regex. The same issue can occur for the declaration of elemental type bound procedures, so it would probably be prudent to expand the regex with both PURE and ELEMENTAL.

Since making this comment 3 days ago, I have been able to confirm (using Javier's example) that adding PURE to the regex fixes the issue.

@bdbaddog
Copy link
Contributor

bdbaddog commented Apr 26, 2019 via email

@bdbaddog
Copy link
Contributor

bdbaddog commented Apr 26, 2019 via email

@jglezplatas
Copy link

jglezplatas commented Apr 26, 2019 via email

@jglezplatas
Copy link

jglezplatas commented Apr 26, 2019 via email

@jglezplatas
Copy link

jglezplatas commented Apr 26, 2019 via email

@mwichmann
Copy link
Collaborator

mwichmann commented Apr 26, 2019 •
edited

Hi! Happy to do test but the problem is that I don't know how apply your changes. Where I find the instructions to follow?

Just FYI, here's one approach. If you have the link to a github Pull Request (PR), you can use that same link and append .patch to use it as a download link to generate a patch file. Git can then apply that with a single command - as long as other things haven't diverged (that's why finding the branch with the PR and making a fork of it is often a safer approach). This is for Linux, do whatever to download the file in case of Windows, maybe open in browser and save?

  1. wget https://github.com/SCons/scons/pull/3359.patch
  2. git apply 3359.patch

@bdbaddog
Copy link
Contributor

It'd be easier to just do the following:

https://github.com/SCons/scons/wiki/GitWorkflows

Clearly you already have a github account. Do you use IRC, if so jump over to #scons and I'll help you through it.

For scons/scons on github.
Then clone that to your sandbox
Then git remote add bdbaddog git@github.com:bdbaddog/scons.git
the git checkout fortran_issue_3135
Make your edits.
git commit -a -m "Update to include pure constructs"
Git push origin
The goto the URL you'll be presented with on how to do pull request.
Make sure the target is my repo and the fortran_issue_3135, and click pull request.
Then follow the URL presented with your

@bdbaddog
Copy link
Contributor

Also please update src/CHANGES to have your name instead of your Github username if you like.

@pdiener
Copy link
Contributor Author

pdiener commented Apr 26, 2019

@bdbaddog. I'll try to fork your branch and update the regex and test. I have not looked at how tests are done before, so have to figure that out first.

@pdiener
Copy link
Contributor Author

pdiener commented Apr 26, 2019

@bdbaddog. I have added PURE and ELEMENTAL to the regex and updated the tests accordingly. I have created a pull request.

@bdbaddog
Copy link
Contributor

@pdiener - Thanks! That wasn't too tough was it? They make the process reasonable. I've merged and once the testing is complete baring issues I'll merge to master and it will make it into the next release!

bdbaddog added a commit that referenced this issue Apr 27, 2019
@bdbaddog
Merge pull request #3359 from bdbaddog/fortran_issue_3135
fdea3fb 
Fix Issue #3135 - Type Bound procedures in Fortran submodules
@bdbaddog
Copy link
Contributor

Merged #3359 - It will show up in next release.

@pdiener
Copy link
Contributor Author

pdiener commented Apr 29, 2019

@bdbaddog. No it wasn't too tough. I'm just not very experienced with the git workflow used here, so I had to take it slow and make sure I did everything correct. I also had not used the scons testing infrastructure before, so I had to spend some time figuring out where things were. Happy to contribute and looking forward to check out the next release. Is there a plan for when the next release will happen?

@bdbaddog
Copy link
Contributor

@pdiener - No hard schedule at this point. Hopefully not too long. A couple bugs I'd like to knock out first.

@jglezplatas jglezplatas mentioned this issue May 6, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@bdbaddog @mwichmann @pdiener @jglezplatas