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

Issue 3541 - Add -oq switch to use fully qualified module for the object file name #1871

Closed
wants to merge 1 commit into from
Closed

Conversation

ghost
Copy link

@ghost ghost commented Apr 8, 2013

@ghost
Copy link
Author

ghost commented Apr 8, 2013

P.S. the reason why I'm replacing periods with underscores is two-fold:

  1. The DMD string operations like to remove extensions from a filename when they attach a new one. This is a small issue that could be worked around.
  2. There might be tools (search tools, parsers, ets) which treat the file name part after the first period as the file extension. Of course these tools might be broken in their assumption, but we can avoid triggering these issues altogether if we use another character such as the underscore.

@ghost
Copy link
Author

ghost commented Apr 8, 2013

I don't understand these permission denied errors, the script works fine locally.

@ghost
Copy link
Author

ghost commented Apr 8, 2013

@9rnsr: Do you know why I'm getting permission denied error on the shell script? I've run chmod +x on it but still get the error.

@9rnsr
Copy link
Contributor

9rnsr commented Apr 8, 2013

Git independently manages file permissions.
You can use this command to change the file permission and commit it.
git update-index --chmod=+x test/compilable/test3541.sh

@ghost
Copy link
Author

ghost commented Apr 8, 2013

Thanks Kenji, it finally greens. I'll wait for reviews.

@WalterBright
Copy link
Member

I don't really understand the point of this. Just use -of to set a specific path/name for the output file. Also, the current scheme of picking output filenames is a confusing mess, with different schemes for each output file kind. I am loathe to add yet another layer on top of that. (see Module::setOutfile() and LibOMF::setFilename() and mars.c line 1499, for starters.

@ghost
Copy link
Author

ghost commented Apr 9, 2013

I don't really understand the point of this. Just use -of to set a specific path/name for the output file.

This is meant to be used with the -c switch to make DMD output multiple object files. Simply using -c alone can't always work because multiple modules can have their object names clash, for example foo.mod and bar.mod will generate a single mod.obj.

Using -op is the alternative, however this spreads out the object files over the source directory. You can also use -op with -od to move the files outside the source directory, however this recreates the entire folder structure for the object files in another directory, which is unnecessary.

-oq is meant to be used with incremental compilation. Incremental compilation is something we're adding to RDMD (and approved by Andrei), and it needs this feature.

For example a sequence of calls by a build tool:

# builder calls dmd to build all modules
dmd -c -oq foo\mod.d bar\mod.d doo\mod.d

# the build tool links the objects
dmd -ofmain.exe foo_mod.obj bar_mod.obj doo_mod.obj

# the user modifies modules bar.mod and doo.mod, the build tool then calls:
dmd -c -oq bar\mod.d doo\mod.d

# the build tool links the objects (foo_mod.obj is old, bar_mod.obj and doo_mod.obj are new)
dmd -ofmain.exe foo_mod.obj bar_mod.obj doo_mod.obj

@timotheecour
Copy link
Contributor

'_' is fragile:
dmd -c -oq foo/mod.d foo_mod.d => name clash!
let's get it right the first time.

alternatives:
a) foo/mod.d => foo.mod => has issues you mentioned but not as bad as name clash above, because '.' shouldn't be allowed in package name if i remember

b) some kind of mangling indicating number of package nesting levels:
foo_mod.d => 1_foo_mod
foo/mod.d => 2_foo_mod

works because package name can't start with a number (right ?)

c) use another character, say @:
foo_mod.d => foo@mod
foo/mod.d => foo_mod

@ghost
Copy link
Author

ghost commented Apr 9, 2013

Good catch. LDC seems to use dots in the filename, I guess to keep compatibility we should probably do the same.

@ghost ghost closed this Apr 9, 2013
@ghost ghost reopened this Apr 10, 2013
@ghost
Copy link
Author

ghost commented Apr 10, 2013

Reimplemented, now using periods instead of underscores. This should keep compatibility with scripts which already used LDC's -oq option.

@ibuclaw: I assume you've probably implemented or know how -oq works in LDC, is this the proper implementation?

@MartinNowak
Copy link
Member

Using -op is the alternative, however this spreads out the object files over the source directory. You can also use -op with -od to move the files outside the source directory, however this recreates the entire folder structure for the object files in another directory, which is unnecessary.

I don't see that this adds much value over -op. Why is it needed?

@s-ludwig
Copy link
Member

One advantage in addition to not creating a full directory structure is that it's much easier to properly place object files in case of the source code being somewhere else than the current directory. Example:

$ pwd
/path/to/build
$ ls
obj/
$ dmd -c -odobj -op ../somefile.d
$ ls
somefile.o
obj/

So instead of in the "obj" folder, everything is now cluttered over the initial folder because the whole path to the source file is used to determine the destination directory. "-oq" on the other hand nicely fills the "obj" folder no matter where the sources come from.

In the VisualD project generator of DUB I have to specify dummy path entries to account for this, which is really just an ugly hack: $ dmd -c -odobj/_dummy -op ../somefile.d

Disclaimer: I didn't actually test this, so it may be slightly off, but you get the idea...

@MartinNowak
Copy link
Member

dmd -c -odobj/_dummy -op ../somefile.d

Indeed pretty ugly.

I don't really understand the point of this.

Just to clarify this because I didn't got it until I tried ldc.
The point of this pull request is to determine object file names from the ModuleDeclaration.
Any good use-cases?
I don't really like the idea because it obfuscates the module file <=> object file mapping.

cat > foo.d << CODE
module bar;
CODE

dmd -c -oq foo.d
ls bar.o

@timotheecour
Copy link
Contributor

What prevents merging this?

I've filed another bug (http://d.puremagic.com/issues/show_bug.cgi?id=12116) and then implemented support for -oq (which works) but then remembered about this thread after 1 Andrej redirected me to http://d.puremagic.com/issues/show_bug.cgi?id=3541.

@andralex
Copy link
Member

@WalterBright are you against this?

@timotheecour
Copy link
Contributor

timotheecour commented Jan 7, 2017

ping: it's been 4 years, this is blocking incremental compilation @ghost @WalterBright @andralex
it also makes it a real pain to seamlessly switch between ldc and dmd (ldc has supported this for a while)

@andralex
Copy link
Member

andralex commented Jan 7, 2017

@timotheecour could you explain how this is a blocker? Simple scripting around dmd -op should be a solid workaround. Most projects and build systems do that.

@timotheecour
Copy link
Contributor

timotheecour commented Jan 8, 2017

@andralex : because dmd -op is hopelessly broken, I just updated https://issues.dlang.org/show_bug.cgi?id=12116 with latest dmd and rdmd HEAD [https://issues.dlang.org/show_bug.cgi?id=12116#c3]

ldc has implemented -oq a long time ago and it works. It makes no sense to have it there and not in dmd.

@timotheecour
Copy link
Contributor

@andralex, will this PR(or another one implementing -oq) be accepted [once made up to date] ?

@andralex
Copy link
Member

andralex commented Jan 9, 2017

@timotheecour @WalterBright is the gatekeeper of the cmdline options, and @MartinNowak is the master of change

@JinShil
Copy link
Contributor

JinShil commented Nov 19, 2017

The audit trail for this pull request is difficult to follow. There were already 2 other pull requests that were merged: #169 and #563

-oq is currently an unrecognized switch in dmd, so I can't understand what the 2 preceding pull requests accomplished.

This issue and PR could use a summary to make review and/or revival more efficient.

@rainers
Copy link
Member

rainers commented Nov 20, 2017

I'm not a fan of -oq: using the module name for the object file makes it difficult for a build system to predict the result of a compilation as it has to parse and analyze the source code in case the module name does not match the file name. That's why I also don't consider adding it to Visual D atm, see also https://issues.dlang.org/show_bug.cgi?id=17796

I agree the dummy folder necessary for dub is ugly, maybe it's simpler to just convert the path to a file name, e.g. by replacing / (plus : and \ on Windows, maybe also .) with some other character.

@ibuclaw
Copy link
Member

ibuclaw commented Nov 20, 2017

I don't really like the idea because it obfuscates the module file <=> object file mapping.

I'm not a fan of that either.

@ibuclaw: I assume you've probably implemented or know how -oq works in LDC, is this the proper implementation?

This switch cannot be implemented in gdc.

@s-ludwig
Copy link
Member

My suggestion would be to generate the object file name from the module file name plus a crc32 or similar of the directory portion. This is what I did for DUB's single-file mode, to keep the overall path length sane and not run into path length issues on Windows.

@ibuclaw
Copy link
Member

ibuclaw commented Nov 20, 2017

@s-ludwig, if build tools already support this, then there's even less of an incentive to do it in the compiler.

@s-ludwig
Copy link
Member

Something still needs to be done for multiple object files output mode, and there is an issue for DMD/COFF when run with the -lib switch and multiple modules in different packages have the same name, but apart from that I'd agree.

@JinShil
Copy link
Contributor

JinShil commented Dec 2, 2017

This doesn't seem to have much support and lacks a champion to carry it forward. I suggest it be closed. Any objections?

@JinShil
Copy link
Contributor

JinShil commented Dec 6, 2017

No objections raised. Closing.

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.

10 participants