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
Adjust README for upstreaming to llvm. #909
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Three references are remaining.
- F18 is written in C++17.
- F18 uses components from LLVM.
-
Build F18
Also, have you checked that it works when invoked as flang?
I guess other references in documentation and source code can be removed later, right?
I don't think there's actually any instructions in the README about invoking the compiler. The binary will still be called f18 unless/until we change that in the CMake files. |
What is the executable that we want users to invoke flang? Ideally, it should be flang and not f18. Do you see any practical difficulties in changing the name of the exe? Also note that @sscalpone created a wrapper script (tools/f18/flang.sh) which seems to be installed in build/tools/f18/bin/flang. So it is possible to invoke as flang. |
Note: The invocation is there in Overview.md in the documentation directory. I guess the invocation as f18/flang can be a separate PR if needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're going to abet the ambiguous use of the name flang
, please add some adjectives or explanatory text so that newcomers aren't confused by the use of the same name for two completely distinct projects.
@klausler @DavidTruby : Would something like the following be OK? |
@kiranchandramohan that text sounds fine to me, if @klausler is ok with it I can change the PR to include that. |
The build instructions are for out of tree builds only, and the headings for those instructions should reflect that.
README.md
Outdated
@@ -34,9 +34,7 @@ read the [style guide](documentation/C++style.md) | |||
and | |||
also review [how flang uses modern C++ features](documentation/C++17.md). | |||
|
|||
## Building Flang |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you still want a section on building flang here so would not remove it completely. The supported C++ compilers bit can stay in the "Building Flang" section (and Carol's patch will beef it up to add more info).
As Flang is not going to be built by default I think we should at least also include instructions on how to add Flang to a build and perhaps link to the LLVM build instructions or mention where to find those.
@@ -47,6 +45,8 @@ The code has been compiled and tested with | |||
clang version 7.0 and 8.0 | |||
using either GNU's libstdc++ or LLVM's libc++. | |||
|
|||
|
|||
## Building Flang out of tree |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would put a little intro here saying what these instructions are for and why you might need to follow them. Probably also recommend building in-tree as far easier.
README.md
Outdated
for more information about Flang. | ||
Flang is a ground-up implementation of a Fortran front end written in modern | ||
C++. It started off as the f18 project (https://github.com/flang-compiler/f18) | ||
at Nvidia with an aim to replace the old flang project |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say "previous flang project" :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to call out NVIDIA, but if you do, please also include the NNSA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed it to not call out anyone in particular, let me know if the new wording is ok!
F18 uses components from LLVM. | ||
## Building Flang out of tree | ||
These instructions are for building Flang separately from LLVM; if you are | ||
building Flang alongside LLVM then follow the standard LLVM build instructions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need this statement in the section above about Building Flang in-tree too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in general if you're building in-tree you're not going to read this README and will just follow the normal build instructions? I can change this though, where would you put it?
Just had a thought while looking over the merging checklist. We should put a few words in here about where to submit issues. During previous emails about this I proposed we keep F18 repository github 'issues' open for this purpose. Can we add a line or two near the top of this README to that effect, including the URL, etc? |
The file should link to a bug tracker, but I don't think that the f18 github repository is the right place unless the fixes to the "issues" are pull requests to the same repository. Why can't the normal llvm bug tracking site be used for bugs being reported and fixed by the community? |
I don’t think there is any reason why it can’t be used.
My reasoning behind proposing F18 issues is
* llvm are discussing a move from bugzilla to github issues<https://mail.google.com/mail/u/0/?zx=wwm6yj2noi7o#search/label%3Adevlist+github+issues%22/FMfcgxwDrtxxqzZcnsgKvSCfGpKfhsvf> so we would be aligning with the direction of travel in that regard (no ETA on that move though.)
* It would also be 0 cost to set up.
* I think the LLVM Bugzilla is not as nice to use as github issues, but that is personal preference.
All weak reasons though – I don’t think there is a compelling reason to chose either approach. Do you have one?
|
Yes, and it's in my original comment. It would be weird and confusing to have bug reporting and tracking activity proceeding on a github repository while fixes and reviews are taking place elsewhere. LLVM has bug tracking and code review systems for LLVM. Use them. |
I think there's some merit in being ahead of the curve here; LLVM is planning on moving to github issues imminently, and it seems to me odd that we would set up and use bugzilla for 2 weeks and then switch back to github issues. |
@DavidTruby Github issues in llvm-project is temporarily disabled until the switch from Bugzilla to github (soon?). |
Oh ok, my mistake! In that case I agree that we should do what the rest of the project does and use Bugzilla until the rest of the project moves. I have no idea how to set that up on the bugzilla side though. |
I've sent an email to the bugzilla admins asking if we can be added on there and what the plan is for the github issues transition; I'll report back on that and update the readme as appropriate. |
Thank you. |
SGTM |
I've posted about the bugzilla to flang-dev as I think it is relevant to more people than will see this PR, you can read the mail here: http://lists.llvm.org/pipermail/flang-dev/2020-April/000269.html |
This changes the references and build instructions for Flang so that they are correct now that F18 has been rechristened Flang and merged with LLVM. Reviewed at: flang-compiler/f18#909
Resolved during upstream merge. |
This changes the references and build instructions for Flang so that they are correct now that F18 has been rechristened Flang and merged with LLVM. Reviewed at: flang-compiler/f18#909
No description provided.