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.

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

LDMD: Append default file extension if -of doesn't contain any #2002

Merged
merged 6 commits into from Mar 9, 2017

Conversation

kinke
Copy link
Member

@kinke kinke commented Feb 17, 2017

Resolves issue #2001.

@thewilsonator
Copy link
Contributor

Would be good to have tests for this.

@kinke
Copy link
Member Author

kinke commented Feb 18, 2017

I wanted to make sure all agree on this (not just for LDMD, but for LDC too) before adding the PITA tests.

@thewilsonator
Copy link
Contributor

Do we not have the ability to do tests for only LDMD? or are you wanting to do this for both and hence waiting?

@ghost
Copy link

ghost commented Feb 18, 2017

the tests should verify this:

command Posix output Windows Output
file.d -of=file file file.exe
file.d -of=file.plugin file.plugin file.plugin
file.d -lib -of=file.plugin file.plugin file.plugin
file.d -lib -of=file file.a file.lib

@kinke
Copy link
Member Author

kinke commented Feb 18, 2017

Do we not have the ability to do tests for only LDMD?

Of course, e.g., dmd-testsuite uses LDMD exclusively.

or are you wanting to do this for both and hence waiting?

This PR does it for both, as it's implemented in LDC, regardless of being invoked via LDMD or not. We could also keep the previous behavior of not tampering with the -of value (for LDC, not LDMD) in case some other tool relied on that, although I think that's highly unlikely as the static/dynamic libs or Windows executables are barely usable without extension.

@bbasile: You missed the DLL output case.

@ghost
Copy link

ghost commented Feb 19, 2017

The corner case is just when a dot is present in the -of argument. Unfortunately I don't speak cpp but I have the feeling that you always add the right extension, which must not be done. The right extension is only added when no extension is already present (row 2 & 3).

@ghost
Copy link

ghost commented Feb 19, 2017

Also a question: You seem make the change in a file that's not specific to LDMD. Does the fix only applies to LDMD ? (it should not affect LDC2).

@kinke
Copy link
Member Author

kinke commented Feb 19, 2017

Don't worry and don't rely on your feelings :], the extension is only added if the filename part doesn't contain a dot (see PR title...). DMD uses the same logic, and FileName::defaultExt() is implemented in the DMD front-end (https://github.com/ldc-developers/ldc/blob/master/ddmd/root/filename.d#L409).

@kinke
Copy link
Member Author

kinke commented Feb 19, 2017

Also a question

I already explained 2 posts further up...

@JohanEngelen
Copy link
Member

I think LDC should do what other compilers do: no automatic file extension adding.

@kinke
Copy link
Member Author

kinke commented Feb 19, 2017

I think LDC should do what other compilers do: no automatic file extension adding.

Which ones did you check? If gcc and clang don't add anything, I very much agree.

@JohanEngelen
Copy link
Member

I checked clang.

@@ -59,6 +59,9 @@ cl::list<std::string> runargs(
"Runs the resulting program, passing the remaining arguments to it"),
cl::Positional, cl::PositionalEatsArgs);

cl::opt<bool> ldmd("ldmd", cl::desc("Invoked by LDMD?"), cl::ZeroOrMore,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you rename the ldmd variable to invokedByLDMD or something else more descriptive?

@kinke kinke changed the title Append default file extension if -of doesn't contain any LDMD: Append default file extension if -of doesn't contain any Mar 4, 2017
@kinke
Copy link
Member Author

kinke commented Mar 4, 2017

https://github.com/ldc-developers/ldc/blob/master/driver/main.cpp#L1031 - shouldn't the default -shared output extension be .dylib instead of .so on OSX (Darwin)?

@kinke
Copy link
Member Author

kinke commented Mar 4, 2017

I'd say let's put this and (adapted) #2021 into 1.2 and then release the first alpha asap.

@JohanEngelen JohanEngelen added this to the 1.2.0 milestone Mar 5, 2017
@@ -16,8 +16,6 @@ if (LDC_WITH_PGO)
if (NOT (LDC_LLVM_VER GREATER 309))
set(PROFRT_EXTRA_LDFLAGS "Ws2_32.lib")
endif()
else()
set(PROFRT_EXTRA_FLAGS "-fPIC -O3")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appropriate optimizations (for MSVC too) are already enforced - we rewrite all CMAKE_C_FLAGS_{DEBUG,MINSIZEREL,RELWITHDEBINFO} with ..._RELEASE in runtime/CMakeLists.txt (as the C parts of druntime/Phobos are always compiled in release too).

@kinke kinke force-pushed the defaultExt branch 2 times, most recently from 308a81a to f16b2a4 Compare March 8, 2017 18:46
Thus providing a generic way to slightly alter some command-line semantics
in DMD compatibility mode.
The POSITION_INDEPENDENT_CODE target property seems to be supported since
CMake 2.8.9, and Ubuntu 12.04 uses 2.8.7.
@kinke kinke merged commit a75c84a into ldc-developers:master Mar 9, 2017
@kinke kinke deleted the defaultExt branch March 9, 2017 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants