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

Don't run gen-expr-info at build time. #9

Closed
vitaut opened this issue Sep 3, 2014 · 22 comments
Closed

Don't run gen-expr-info at build time. #9

vitaut opened this issue Sep 3, 2014 · 22 comments

Comments

@vitaut
Copy link
Contributor

vitaut commented Sep 3, 2014

Since expressions don't change often, it's probably OK to add expr-info.cc to the repo and possibly get rid of gen-expr-info.

@vitaut
Copy link
Contributor Author

vitaut commented Sep 26, 2014

expr-info.cc is now in the repo: 4c447a2

Need to also look into arithchk.

@tkelman
Copy link
Contributor

tkelman commented Sep 26, 2014

Thanks, that helps. I guess I'd need to patch out the add_custom_command line and take the ${MP_BINARY_DIR}/ out of MP_EXPR_INFO_FILE to use the checked-in version of expr-info.cc instead of regenerating a new one at build time. Not too bad. Making a build configuration flag to switch this behavior would be useful.

If arithchk is more difficult I can generate arith.h with an offline build, once for 32 bit and once for 64 bit and include the results as a supplementary source file. Extra work but an okay stopgap.

@vitaut
Copy link
Contributor Author

vitaut commented Sep 26, 2014

Oops, I forgot to commit CMakeLists.txt. MP_EXPR_INFO_FILE path should be correct now. Not sure what's the purpose of the flag though. The file will be regenerated (only) if any of the dependencies change. Do you want to be able to disable this?

@tkelman
Copy link
Contributor

tkelman commented Sep 26, 2014

I think f2f5d67 should be fine. Will have to see what happens when I delete BuildRequires: wine from here. I'll let you know if any patches are required.

@vitaut
Copy link
Contributor Author

vitaut commented Sep 26, 2014

Thanks. I'll have a look if I can get rid of arithchk at least for common case (x86/amd64).

@tkelman
Copy link
Contributor

tkelman commented Oct 7, 2014

I should've tested this before you tagged a new version, ah well. Turns out I did need to delete the two add_custom_command calls in order to build a cross-compile without wine, using the checked-in version of expr-info.cc and a pre-generated copy of arith.h.

@vitaut
Copy link
Contributor Author

vitaut commented Oct 7, 2014

I see what the problem is. expr-info.cc depends on gen-expr-info which is not in the repo. So building from scratch generates gen-expr-info and causes expr-info.cc regeneration.

Unfortunately I don't see a safe way to get rid of the dependency between expr-info.cc and gen-expr-info. Another options is to disable the command that generates expr-info.cc if cross-compilation detected and issue an error if it should be re-generated.

@tkelman
Copy link
Contributor

tkelman commented Oct 8, 2014

Or, I guess, as things stand right now you just have a dependency on wine for cross-compilation. That means you can't compile win64 binaries from 32 bit linux, but that might not be a very common setup. To avoid the dependency on wine, I have to carry a patch to disable the add_custom_command calls and supply my own pregenerated arith.h. Not that bad.

@vitaut
Copy link
Contributor Author

vitaut commented Oct 8, 2014

As it turned out, the option that I suggested in my previous comment (implemented in 29f770d) doesn't work because after a git clone the modification times are slightly different.

I can disable the add_custom_command calls when cross compiling if you like.

@tkelman
Copy link
Contributor

tkelman commented Oct 9, 2014

I don't think it's very high-priority. Even if gen-expr-info is figured out, there's still arith.h which varies a little bit from platform to platform. Common cases would probably not be that hard to cover though, especially the cases where cross compilation is most often used.

@vitaut
Copy link
Contributor Author

vitaut commented Dec 17, 2014

I've ported most arithmetic checks to CMake (18382c8). These are used when cross compiling (9d15fad) and if target platform is x86 or x86_64.

@tkelman
Copy link
Contributor

tkelman commented Dec 17, 2014

Very cool, I'll have to give that a try.

@vitaut
Copy link
Contributor Author

vitaut commented Jan 6, 2015

Hi @tkelman, did you have a chance to try this?

@tkelman
Copy link
Contributor

tkelman commented Jan 6, 2015

okay I just tried at 3cd123f and it's still trying to run arithchk through wine:

[   99s] make -f src/asl/CMakeFiles/asl-core.dir/build.make src/asl/CMakeFiles/asl-core.dir/depend
[   99s] make[2]: Entering directory '/home/abuild/rpmbuild/BUILD/mp-3cd123f67fb368cc3ecf49ddbd0c461ac3b750d7'
[  100s] /usr/bin/cmake -E cmake_progress_report /home/abuild/rpmbuild/BUILD/mp-3cd123f67fb368cc3ecf49ddbd0c461ac3b750d7/CMakeFiles 
[  100s] [  5%] Generating arith.h
[  100s] cd /home/abuild/rpmbuild/BUILD/mp-3cd123f67fb368cc3ecf49ddbd0c461ac3b750d7/src/asl && wine /home/abuild/rpmbuild/BUILD/mp-3cd123f67fb368cc3ecf49ddbd0c461ac3b750d7/bin/arithchk.exe > arith.h
[  100s] /bin/sh: wine: command not found
[  100s] src/asl/CMakeFiles/asl-core.dir/build.make:56: recipe for target 'src/asl/arith.h' failed
[  100s] make[2]: *** [src/asl/arith.h] Error 127
[  100s] make[2]: Leaving directory '/home/abuild/rpmbuild/BUILD/mp-3cd123f67fb368cc3ecf49ddbd0c461ac3b750d7'
[  100s] CMakeFiles/Makefile2:404: recipe for target 'src/asl/CMakeFiles/asl-core.dir/all' failed
[  100s] make[1]: *** [src/asl/CMakeFiles/asl-core.dir/all] Error 2

@vitaut
Copy link
Contributor Author

vitaut commented Jan 6, 2015

Thanks for trying it out. You might need to set CMAKE_SYSTEM_PROCESSOR CMake variable for this to work:

cmake -DCMAKE_SYSTEM_PROCESSOR=x86_64 ...

@tkelman
Copy link
Contributor

tkelman commented Jan 6, 2015

Ah thanks, trying that now. And presumably it's x86 for 32 bit?

@vitaut
Copy link
Contributor Author

vitaut commented Jan 6, 2015

Yeah, although it doesn't matter at the moment whether you use x86 or x86_64. The target pointer size is detected using different means.

@tkelman
Copy link
Contributor

tkelman commented Jan 7, 2015

Looks really promising, I think we can close this resolved fixed. I'll leave another comment or open a new issue if I have any future problems.

Thanks, and belated happy new year!

@vitaut
Copy link
Contributor Author

vitaut commented Jan 7, 2015

Cool, closing this issue then.
Happy new year to you too!

@vitaut vitaut closed this as completed Jan 7, 2015
@tkelman
Copy link
Contributor

tkelman commented Jan 7, 2015

Oh yeah, I guess if you feel so inclined you could tweak the Travis config to not install wine until it's time to run the tests, since it's no longer needed at build time.

@tkelman
Copy link
Contributor

tkelman commented Jan 7, 2015

And see http://docs.travis-ci.com/user/multi-os/ if you might want to enable Travis for OSX. Would have to tweak things around a bit since you use brew instead of apt-get and would want to disable the cross-compile on Mac.

@vitaut
Copy link
Contributor Author

vitaut commented Jan 7, 2015

Good idea. With wine installation postponed compile errors may appear earlier.
As for OSX build, I do plan to enable it. Already did it for another repo (cppformat).

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

No branches or pull requests

2 participants