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

-o is ignored for C files #6475

Closed
vicuna opened this issue Jun 30, 2014 · 23 comments
Closed

-o is ignored for C files #6475

vicuna opened this issue Jun 30, 2014 · 23 comments
Labels
Milestone

Comments

@vicuna
Copy link

vicuna commented Jun 30, 2014

Original bug ID: 6475
Reporter: @yallop
Assigned to: @whitequark
Status: resolved (set by @gasche on 2016-06-14T15:40:54Z)
Resolution: fixed
Priority: normal
Severity: major
Target version: 4.03.1+dev
Fixed in version: 4.04.0 +dev / +beta1 / +beta2
Category: ~DO NOT USE (was: OCaml general)
Tags: patch
Related to: #5890 #6218
Monitored by: @whitequark @gasche @hcarty

Bug description

The -o option is ignored when compiling C files, which makes it slightly awkward to write object files to a directory other than the current directory.

For example, the following command

ocamlc -c -o build/code.cmo src/code.ml

creates 'build/code.cmo' as expected, but the corresponding command for a C file

ocamlc -c -o build/code.o src/code.c

creates 'code.o' in the current directory.

File attachments

@vicuna
Copy link
Author

vicuna commented Oct 23, 2014

Comment author: @whitequark

I should add that ocamlbuild should be fixed as well when this issue is resolved. It currently uses mv to move the .o from the current directory.

@vicuna
Copy link
Author

vicuna commented Oct 24, 2014

Comment author: @yallop

There's a patch by @vbgl here:

#103

@vicuna
Copy link
Author

vicuna commented Dec 21, 2014

Comment author: @gasche

whitequark, could you write a test case in the ocamlbuild testsuite to make sure your ocamlbuild patch works as expected? There already is some very light output-obj testing in internal.ml.

@vicuna
Copy link
Author

vicuna commented Dec 21, 2014

Comment author: @whitequark

I have attached a new patch that contains:

  • @vbgl's fix
  • changes to ocamlbuild
  • ocamlbuild test
  • changes to Changes

@vicuna
Copy link
Author

vicuna commented Dec 21, 2014

Comment author: @gasche

I merged the (very nice!) patch, but in trunk only rather than in the stable branch. My reasoning is that other build systems may have workarounds for this issue (we had to fix ocamlbuild) which may break now that it is fixed.

(We don't know yet whether there will be another minor release, but Damien seems to be thinking about it.)

Thanks to all of you for the report and bugfixes.

@vicuna
Copy link
Author

vicuna commented Dec 21, 2014

Comment author: @whitequark

I believe this can be merged in stable, because the patch does not change the behavior without -o, and other buildsystems are unlikely to pass an option that does nothing. So the workarounds will still work.

@vicuna
Copy link
Author

vicuna commented Dec 21, 2014

Comment author: @whitequark

(In other words we did not have to fix ocamlbuild, I just thought it would be much cleaner.)

@vicuna
Copy link
Author

vicuna commented Dec 21, 2014

Comment author: @gasche

Okay, will merge in 4.02 at a later date.

@vicuna
Copy link
Author

vicuna commented Dec 27, 2014

Comment author: @gasche

Merged in 4.02.

@vicuna
Copy link
Author

vicuna commented Feb 20, 2015

Comment author: @damiendoligez

There is a problem with this change: it breaks (at least) ocamlnet because the ocamlnet config script does this:

ocamlc -custom -o t tend.c t.ml

And with this patch, the C compiler puts its output in t instead of putting it in tend.o where the OCaml compiler expects it.

The long-term solution is for users to be aware that the order of arguments is important and to write instead:

ocamlc -custom tend.c t.ml -o t

I wouldn't mind doing this in trunk and documenting it properly ("sets the name of the output file for the next source file, or for the whole compilation if it appears on the command line later than all sources files") but for 4.02 I think it's too much incompatibility, so we should roll back.

We should also probably reset the -o argument as soon it has been used for one source file, since it doesn't make sense to overwrite a file you have just generated.

Thoughts?

@vicuna
Copy link
Author

vicuna commented Feb 20, 2015

Comment author: @whitequark

No, I think that the current behavior is actually wrong. There is no reason whatsoever to set the output file name for one of the intermediate build products. Indeed, the -o option should always set the filename for the whole compilation.

@vicuna
Copy link
Author

vicuna commented Feb 21, 2015

Comment author: @gasche

I just reverted (4.02@15863) the 4.02 commit. The behavior in trunk is still problematic, so I'll revert it as well if don't converge on a solution.

@vicuna
Copy link
Author

vicuna commented Feb 23, 2015

Comment author: @damiendoligez

@whitequark, I thought we had decided it would be nice to be able to do:

ocamlopt -c -o /tmp/foo.o foo.c -o /tmp/bar.cmx bar.ml

Or maybe that's overkill because no build system will do that. In that case, we still want -o to work if there's only one source file and the -c option is also given.

@vicuna
Copy link
Author

vicuna commented Feb 23, 2015

Comment author: @whitequark

I think that's overkill and is very confusing. No other compiler in existence does that, and even few Unix tools do.

Indeed; with -c, that would qualify as the end result of compilation.

@vicuna
Copy link
Author

vicuna commented Jan 21, 2016

Comment author: nevor

The commit seems to have been reverted after the merge into trunk. The current behavior (still) breaks library generation from mixed C/OCaml files as described by doligez.

@vicuna
Copy link
Author

vicuna commented Jan 27, 2016

Comment author: @alainfrisch

I'm raising the Severity, since this is an actual regression from the previous release. We should either revert the change or find a proper solution.

@vicuna
Copy link
Author

vicuna commented Feb 2, 2016

Comment author: @alainfrisch

I propose to revert this today, unless someone objects to it (or, better, fixes the issue).

@vicuna
Copy link
Author

vicuna commented Feb 2, 2016

Comment author: @whitequark

I object to reverting. Is it hard to make a change so that the name for the whole compilation is set? I would do it, but I am trying to move internationally right now, so a bit busy here. I can do it in about three days.

@vicuna
Copy link
Author

vicuna commented Feb 2, 2016

Comment author: @alainfrisch

Is it hard to make a change so that the name for the whole compilation is set?

I don't know, I did not look at it. I only observe that the current behavior can introduce regression in the wild (concretely, at LexiFi, we had to fix on Makefile locally after synchronizing with trunk) and that it should be fixed before the release. I'll let Damien decide on what to do.

@vicuna
Copy link
Author

vicuna commented Feb 6, 2016

Comment author: @whitequark

Damien, any thought on this? I will be able to spend some time on this soon.

@vicuna
Copy link
Author

vicuna commented Feb 8, 2016

Comment author: @damiendoligez

What the compiler should do is clear:

  • if -o is given but not -c (set the file name for the final product only)
  • if -o and -c are given with only one source file (set the file name for the sole output file)

What is less clear is what to do when -o and -c are given, and several source files are present. If you can make that an error (similar to ocamlopt -shared without -o) I think we're good.

More generally, you want to trigger an error instead of overwriting an output file that was just written by the same command.

4.03.0 will be released within one month, so you should propose a patch within the next few weeks. And make it as simple as possible, so we can be sure it doesn't break something else.

@vicuna
Copy link
Author

vicuna commented Feb 9, 2016

Comment author: @whitequark

Please see #464.

@vicuna
Copy link
Author

vicuna commented Jun 14, 2016

Comment author: @gasche

I just merged #2876. Anyone interested in understanding the impact should read whitequark's excellent commit message at:
da56cf6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant