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
Name clash with memory.h header file with the Android NDK #5887
Comments
Comment author: vouillon I have uploaded a tentative script to move most include files in byterun to byterun/caml. The file instruct.h and the generated files jumptbl.h, opnames.h, and version.h are left in byterun. They could be moved as well but several makefiles have to be patched for that. "make world" and "make opt" works, but I see now that myocamlbuild.ml and some files in the build directory need to be updated as well. Also, the .depend.nt files are not updated. |
Comment author: Camarade_Tux I've uploaded 0001-Move-byterun-.h-to-byterun-caml.patch.gz . I've used your script to create it and also moved {instruct,jumptbl,opnames,version}.h and updated myocamlbuild.ml. Build using world.opt works. I haven't touched the .depend.nt files. The patch is huge (8000 lines added and removed). Half of that is because of the .depend files. Another half is the .h files themselves (since being moved counts as additions and deletions) and the renaming in the #include directives of the .c files. My git commit message (which is in the patch file too) is as follows:
|
Comment author: meyer My only comment: if we plan to move the files around - which I think is a good idea, then we should be using svn machinery for that e.g. svn mv command, as svn is aware about files that are moved, unlike git which just track contents. But many thanks for the patches, it really helps to know what should be done. |
Comment author: meyer
Don't need to care about myocamlbuild.ml at the moment, I don't know if anybody builds OCaml with ocamlbuild at all, and we plan to just remove these files from the tree. As for .depend.nt, it's crucial that somebody would test it on Windows. Thanks |
Comment author: Camarade_Tux Well, I can't "svn mv" myself but here's the diffstat I got in git here, "=>" shows the renames: .depend | 14 +- |
Comment author: Camarade_Tux I've found two issues so far: In byterun/debugger.c: In byterun/win32.c: These should be <io.h>, not <caml/io.h>. :-) EDIT: |
Comment author: Camarade_Tux Another issue. From tools/cleanup-header: These regular expressions need to be changed to match "../../" instead of only "../". |
Comment author: @whitequark What is the status of this patchset? |
Comment author: Camarade_Tux Also, re my comment above, the following needs can be run after applying the patches: sed -i 's;\.\.\/;\0\0;g' tools/cleanup-header (you can copy-paste this directly after the application of the patches) |
Comment author: @whitequark It would be very nice to have this integrated in 4.02.2, or maybe 4.03. I could assist with this if someone lists what is wrong with the current state of the patch so that I could fix it. |
Comment author: @gasche I could apply the patch if you had something ready-to-apply. |
Comment author: @whitequark @gasche, I have attached a set of three patches. The first patch is "0001-rename.patch". You should not apply it. Rather you should use svn mv, I believe:
The second patch is handwritten "0002-fix.patch" (an adaptation of adrien's patch). The third patch is the result of:
Both of these can be applied. |
Comment author: @whitequark By the way, if you could apply it to 4.02.2, that would be great. 4.02.2 will have excellent support for cross-compiling if this and another issue, which I will file shortly, will be fixed. |
Comment author: @gasche Trying to apply the patch on the current trunk gives me a test failure (apparently some tests in testsuite/ refer to .h files directly). Could you provide the extra patch to fix theses, or merge it into 0002-fix? The testsuite is run by the target |
Comment author: @whitequark I've attached the patch that fixes all failures related to header files. |
Comment author: @gasche There typing-* failures are already fixed in trunk, they were caused by your *_ascii patches :-) |
Comment author: @gasche I applied the patchset in trunk; I'm waiting to see how the continuous-integration reacts (in particular on Windows builds). |
Comment author: @whitequark Looks like everything is OK--this PR together #4175 are resolved. (I'm not sure whether it is appropriate to mark the PRs as resolved or closed, so I will leave this to someone else.) |
Comment author: @gasche I'm not decided on whether I want to propagate the change to the 4.02 branch. Pros:
Cons:
|
Comment author: @damiendoligez @gasche, could you create a patch for the 4.02 branch? I'll test it against OPAM packages. I'd like to get it in 4.02.2 if it doesn't break anything in practice, as @whitequark is eager to have it. |
Comment author: @gasche I'm sorry, but I won't have time to work on this soon. Would anyone (whitequark, Jérôme, Adrien ?) be willing to do the work of backporting the patches applied to trunk? It's not an issue if they are large diff rather than proper "mv" commands, during the testing phase at least. I had to apply a handful of tweaks after the patches as suggested in this PR. The additional patches are the following: (The "big" patch is: |
Comment author: @whitequark I did so. |
Comment author: @gasche Thanks ! |
Comment author: @damiendoligez I'll test the patch on OPAM and report here. |
Comment author: @damiendoligez I'm not seeing any impact on the 770 OPAM packages that compile on my machine, so green light for this change for 4.02.2. |
Comment author: @gasche I'm in the process of applying the change to 4.02 as well. The commits have been done. Hopefully everything goes well and we can close this issue. |
Comment author: @gasche There seems to be a problem on Windows builds (at least the CI complains), but I don't have a Windows machine at hand so I can't test/debug the problem. Volunteer help warmly welcome -- otherwise it will just wait for a few extra weeks, I suppose. |
Comment author: @alainfrisch I haven't build able to reproduce the problem on a fresh checkout of 4.02 (tested with the msvc32 port). What's weird is that byterun/.depend in the repository doesn't list fail.h as a dependency for win32.o (but rather caml/fail.h as expected). |
Comment author: @damiendoligez The patch is applied and the windows builds have fixed themselves. @gasche, can we close this PR? |
Original bug ID: 5887
Reporter: vouillon
Assigned to: @gasche
Status: closed (set by @xavierleroy on 2017-02-16T14:14:12Z)
Resolution: fixed
Priority: high
Severity: minor
Version: 4.00.1
Target version: 4.02.2+dev / +rc1
Fixed in version: 4.02.2+dev / +rc1
Category: runtime system and C interface
Tags: patch
Has duplicate: #4175
Related to: #6266
Monitored by: @whitequark @protz Camarade_Tux
Bug description
In the Android NDK, the stdlib.h header file includes memory.h.
When compiling the native runtime and the libraries in otherlibs, it's the OCaml memory.h file that gets included. The compilation then breaks when misc.h is included, as it includes stdlib.h before providing definitions need by memory.h.
A work-around is to temporarily define CAML_MEMORY_H before including stdlib.h
(see patch) in misc.h. As the memory.h system header file is empty, this is safe to do.
A better fix, which would also fix the name clash on io.h under Windows (#4175),
would be to move OCaml header files into a subdirectory "caml" of directory "byterun". I could implement that if there is some interest.
File attachments
The text was updated successfully, but these errors were encountered: