This is a fix for the command line syntax error while building LTO when using MinGW.
Details
Details
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
Comment Actions
In general I don't think I should be the one to review this patch because it seems to be Windows specific, and I don't really do dev on Windows. However, since you ping'd me, see my comment below.
| cmake/modules/AddLLVM.cmake | ||
|---|---|---|
| 96 ↗ | (On Diff #14024) | By moving the definition of export_file_backslashes into this else clause it won't be defined on Cygwin, so I don't think this will build. I also think this should probably actually be in a separate if block for "if (WIN32 and not CYGWIN)". My reasoning for that (which may be wrong, so take it with some salt) is you're in a block now that is basically "if (not Darwin and not LLVM_HAVE_LINK_VERSION_SCRIPT)". In practice that seems to mean WIN32, but I don't think that is a hard requirement. In particular since this is really related to linker features it is plausible that in the near future lld may not require a link version script, and you could use it on any platform. |
Comment Actions
It has to be moved there beause Cygwin uses forward slashes (and cat), as it is a full-fledged posix environment.
Assuming WIN32 unconditionally should be fine, as by default the whole block uses type instead of cat. Which is Windows only.
Comment Actions
Assuming Posix "cat" exists by default is no better than assuming "type" exists by default, especially in a situation that, as of right now, only applies to Windows environments. Furthermore I'd want to keep changes that are not related to the problem itself to a minimum/have them in a later refactoring-only patch.
About the second part, I misunderstood what you meant. It currently would not work on Cygwin. I attached a new version of my patch to this message. I'm unsure what the further procedure is as this is my first patch submitted to LLVM.
Comment Actions
To answer your question about the process for submitting patches, the developer process is documented here:
http://llvm.org/docs/DeveloperPolicy.html
New community members are required to submit patches for review, have them approved, and get someone else to commit them. Once you have a track record of submitting quality patches you can ask for commit access.
With regard to this patch, I'm not satisfied with it in its current state.
You have two options. Either you can adapt the patch based on the feedback I've provided, or you can get someone else in the community to review and accept the patch. I admit I am not the most qualified person to review this patch.
This comment was removed by jsroemer.
Comment Actions
Owen,
I didn't mean to imply he should circumvent me. I was just trying to re-iterate, that this patch isn't exactly my area of expertise.
Johannes,
I get that you put in updates to the patch, but your updates (which were attached earlier) don't actually address my original feedback. This may just be a miscommunication, so let me clarify. Your changes are inside a conditional block that is essentially "if (not darwin and not LLVM_HAVE_LINK_VERSION_SCRIPT)". While in practice it may be equivalent, it is not actually the same as "if (Windows)".
That is why I think this patch needs to be updated to something more similar to:
diff --git a/cmake/modules/AddLLVM.cmake b/cmake/modules/AddLLVM.cmake index 81b21fb..9ebeca4 100644 --- a/cmake/modules/AddLLVM.cmake +++ b/cmake/modules/AddLLVM.cmake @@ -85,17 +85,20 @@ function(add_llvm_symbol_exports target_name export_file) else() set(native_export_file "${target_name}.def") - set(CAT "type") - if(CYGWIN) - set(CAT "cat") + set(CAT "cat") + set(export_file_nativeslashes ${export_file}) + if(WIN32 AND NOT CYGWIN) + set(CAT "type") + # Convert ${export_file} to native format (backslashes) for "type" + # Does not use file(TO_NATIVE_PATH) as it doesn't create a native + # path but a build-system specific format (see CMake bug + # http://public.kitware.com/Bug/print_bug_page.php?bug_id=5939 ) + string(REPLACE / \\ export_file_nativeslashes ${export_file}) endif() - # Using ${export_file} in add_custom_command directly confuses cmd.exe. - file(TO_NATIVE_PATH ${export_file} export_file_backslashes) - add_custom_command(OUTPUT ${native_export_file} COMMAND ${CMAKE_COMMAND} -E echo "EXPORTS" > ${native_export_file} - COMMAND ${CAT} ${export_file_backslashes} >> ${native_export_file} + COMMAND ${CAT} ${export_file_nativeslashes} >> ${native_export_file} DEPENDS ${export_file} VERBATIM COMMENT "Creating export file for ${target_name}")
This patch doesn't merely make 'cat' the default instead of 'type', it actually handles the only case you should ever use 'type', which is if you are on Win32 and not using Cygwin. Do you understand the distinction I'm trying to make?
Comment Actions
Although I still disagree with the notion that "cat" is in any way more standard than "type", I uploaded a new patch which does so. And if we are going to assume POSIX by default, we should do it right, so I updated how the corresponding linker flag is added as well.
Comment Actions
Johannes,
Since you don't have commit access, I will land this patch for you in a few minutes. Congratulations on having your first patch accepted.
Welcome to the LLVM community!
-Chris