Page MenuHomePhabricator

[flang] Make the bash script print short info whenever used
AbandonedPublic

Authored by awarzynski on May 3 2022, 5:24 AM.

Details

Summary

The flang bash script is frequently confused for Flang's compiler
driver. While its purpose and limitations are documented [1], this is
not always easy to find.

With this patch, it should be clear that flang is just a wrapper script
(rather than a driver) whenever it is used.

[1]
https://github.com/llvm/llvm-project/blob/main/flang/docs/FlangDriver.md#the-flang-script

Diff Detail

Event Timeline

awarzynski created this revision.May 3 2022, 5:24 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
awarzynski requested review of this revision.May 3 2022, 5:24 AM

Based on https://github.com/llvm/llvm-project/issues/53420#issuecomment-1115969047, I feel that it would be very helpful to make the purpose of flang a bit clearer. WDYT?

ekieri added a comment.May 3 2022, 1:00 PM

Thanks for doing this, would be great if we can reduce the confusion generated by this script. I have almost never used this script myself, and is not really sure what it is for, so I am perhaps not in the best position to comment. But here are my thoughts, for what it is worth:

  • The lines are longer than 80 characters, which might annoy some people. Admittedly, this is hard to avoid with an 89 char URL, which really deserves to be there.
  • The message sounds like a quite strong discouragement of using the script. If that is the intention, why not also/instead rename flang-new as flang and the bash script as flang-old or flang-wrapperscript or something in that direction? (This comment is mostly a question of "was this your intention?" rather than a real suggestion to rename the driver and script - I do not have much of an opinion in any direction there.)
  • Does any flang developer use this script for actual work? - if so, it would be good to get their input. I guess it could be used to test the frontend on large 3rd party projects, and if you do that you would get hundreds of messages like that. But probably nobody has such a workflow? And anyway. I do not see a way of making the message much shorter without losing important information.

Here's a different way of wording it (also breaking the 80 char barrier...). More to contrast your version with something different than as a proposal, but feel free to use or rephrase (parts of) it if you like it.

================================================================================
This Flang wrapper script will run semantic checks and defer compilation to get_external_fc_name(). See
https://github.com/llvm/llvm-project/blob/main/flang/docs/FlangDriver.md#the-flang-script
for more details. The Flang compiler driver is flang-new.
================================================================================

Thanks for the review, @ekieri !

  • The lines are longer than 80 characters, which might annoy some people. Admittedly, this is hard to avoid with an 89 char URL, which really deserves to be there

👍 It wouldn't be the first violation of the 80 character rule in this script, so perhaps that's not so bad?

  • The message sounds like a quite strong discouragement of using the script. If that is the intention

Based on various GitHub issues/Discourse threads, it is clear that this wrapper script is a source of a lot of confusion. People tend to use it without realising that that's not Flang's compiler driver. That's not really that surprising - the name is quite misleading :) My main intention is to reduce this confusion.

why not also/instead rename flang-new as flang and the bash script as flang-old or flang-wrapperscript or something in that direction?

I agree that that would be much better and that's the ultimate goal :)

  • Does any flang developer use this script for actual work? - if so, it would be good to get their input.

This wrapper script was introduced to facilitate the transition from f18 (the old driver) to flang-new. I don't use nor require it myself, but I do know that @sscalpone uses it for testing. And we agreed to keep here until Steve can switch to flang-new. This might be the right time to revisit that discussion :) In any case, given that this script is important for Steve, we should definitely wait for him to take a look before merging (just like you suggested).

Here's a different way of wording it (also breaking the 80 char barrier...).

Thanks, sounds much better, I'll use that if you don't mind!

Sure, if you find my draft message useful, please go ahead. Whether you use it or not, consider me satisfied. But yes, please wait for Steve.

awarzynski abandoned this revision.May 17 2022, 7:25 AM

Abandoning in favor of https://reviews.llvm.org/D125788 :)

Thanks for the review @ekieri !