Page MenuHomePhabricator

[flang][driver] Refine tests for module search directories
ClosedPublic

Authored by awarzynski on Feb 22 2021, 8:25 AM.

Details

Summary

This patch refactors include-module.f90:

  • rename the test file as use-module.f90 to better highlight which driver feature is being tested
  • move tests for diagnostics to use-module-error.f90 (it tests that -J/-module-dir can only be used once)
  • make sure that f18 is tested when FLANG_BUILD_NEW_DRIVER is set to Off
  • add tests for when all module files are successfully discovered and loaded

With this patch, there should be a clear separation into 3 scenarios in
use-module.f90:

  • Everything is OK
  • One module file wasn't found (missing include path for basictestingmoduletwo.mod)
  • Two module files are found, but the test requires basictestingmoduleone.mod from both Inputs and Inputs/module-dir. Only the latter is found.

Depends on: D97164

Diff Detail

Event Timeline

awarzynski created this revision.Feb 22 2021, 8:25 AM
awarzynski requested review of this revision.Feb 22 2021, 8:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 22 2021, 8:25 AM

make sure that f18 is tested when FLANG_BUILD_NEW_DRIVER is set to Off

I don't think any of the tests in flang/test/Flang-Driver are run when that flag is Off, due to the configuration in flang/test/lit.cfg.py. That's why I added flang/test/Driver/write-module.f90 in D97164. Those tests are run in either configuration.

flang/test/Flang-Driver/Inputs/module-dir/basictestmoduleone.mod
3

Rather than check in the .mod files (whose format can change at any time) it would be better to check in the Fortran source and compile it for tests that need it. It would make the tests less fragile.

flang/test/Flang-Driver/use-module-error.f90
9

For tests like this that require the new driver, it would be better to use %flang rather than %flang-new. Otherwise in ten years it will still say %flang-new and people will wonder what's new about it.

16

Same here: use %flang_fc1.

Use %flang and %flang_fc1 instead of %flang-new.

awarzynski marked 2 inline comments as done.Feb 22 2021, 10:31 AM

make sure that f18 is tested when FLANG_BUILD_NEW_DRIVER is set to Off

I don't think any of the tests in flang/test/Flang-Driver are run when that flag is Off, due to the configuration in flang/test/lit.cfg.py. That's why I added flang/test/Driver/write-module.f90 in D97164. Those tests are run in either configuration.

I assumed that the reason for a separate file in D97164 was -module and -fparse-only (which are not available in the new driver). I should've asked. I think this is all fine and is a good indicator that it's time to update flang/test/lit.cfg.py: https://reviews.llvm.org/D97207.

flang/test/Flang-Driver/Inputs/module-dir/basictestmoduleone.mod
3

I agree that we should aim for something more reliable/stable. Could that be a separate patch though?

flang/test/Flang-Driver/use-module-error.f90
9

Old habits. Updated.

tskeith added inline comments.Feb 23 2021, 9:07 AM
flang/test/Flang-Driver/Inputs/module-dir/basictestmoduleone.mod
3

Yes, that's fine.

awarzynski marked an inline comment as done.

Rebase on top of main

tskeith accepted this revision.Mar 8 2021, 9:55 AM

This is more a comment for D97207, but it's related to -module-dir: We probably don't need flang/test/Driver/write-module.f90 any more if it's tested in flang/test/Flang-Driver. In fact, I think we should get rid of all of the tests in flang/test/Driver and rename flang/test/Flang-Driver to flang/test/Driver.

This revision is now accepted and ready to land.Mar 8 2021, 9:55 AM

This is more a comment for D97207, but it's related to -module-dir: We probably don't need flang/test/Driver/write-module.f90 any more if it's tested in flang/test/Flang-Driver. In fact, I think we should get rid of all of the tests in flang/test/Driver and rename flang/test/Flang-Driver to flang/test/Driver.

I agree, please see https://reviews.llvm.org/D98257.