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

cmake change default build directory, configure script, Fedora support #460

Merged
merged 22 commits into from Jun 12, 2015

Conversation

dagar
Copy link
Contributor

@dagar dagar commented Apr 22, 2015

Tried to hijack #441 for this, but couldn't once I deleted the original branch.

This changes the build directories to
$(SRC_DIR)/build/Debug
$(SRC_DIR)/build/Release
$(SRC_DIR)/build/Debug-gcc

I think the usual complaint about polluting the source directory is when the object files are right next to source, but I guess we'll see. It's easy to override BUILD_DIR.

@dagar dagar force-pushed the cmake-default branch 3 times, most recently from 0e4f089 to 49e2cf7 Compare April 30, 2015 23:59
@dagar dagar changed the title change default build directories cmake change default build directory and configure script May 1, 2015
@dagar
Copy link
Contributor Author

dagar commented May 1, 2015

@kmod I fixed the configure script here. For reasons I don't fully understand lz4 using CheckTypeSize causes a conflict between our imported version and the older system version. I resolved it by renaming the imported newer version to CheckTypeSizeof.

@dagar
Copy link
Contributor Author

dagar commented May 1, 2015

Integration tests failing. Not sure how to get the unordered_map size into this compilation.

pycrypto_test.py    FAILED (Exited with code 1 (expected code 0))

warning: PCTBuildPy: byte-compiling is disabled, skipping.

In file included from /home/dagar/pyston-build/from_cpython/Include/Python.h:53:0,
from src/pycrypto_common.h:27,
from src/_fastmath.c:29:
/home/dagar/pyston-build/from_cpython/Include/object.h:458:24: error: ‘SIZEOF_UNORDEREDMAP’ undeclared here (not in a function)
char _dep_getattrs[SIZEOF_UNORDEREDMAP + sizeof(void *)];
^
error: command 'gcc' failed with exit status 1
Traceback (most recent call last):
File "/home/dagar/git/pyston/test/integration/pycrypto_test.py", line 26, in :
subprocess.check_call([sys.executable, "setup.py", "build"], stdout=devnull)
File "/home/dagar/pyston-build/from_cpython/Lib/subprocess.py", line 540, in check_call:
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['/home/dagar/pyston-build/pyston', 'setup.py', 'build']' returned non-zero exit status 1

@dagar
Copy link
Contributor Author

dagar commented May 1, 2015

Cmake could copy object.h and dictobject.h to the build directory and insert the actual size, but I'm not sure how I feel about that.

@kmod
Copy link
Collaborator

kmod commented May 1, 2015

Ah ok, sounds like we should switch from having our own config.h to using the existing pyconfig.h. That entire file should be generated, so we'll eventually want to be using this process to generate it anyway, and it makes sense as a place for SIZEOF_UNORDEREDMAP to live. Plus it lives in from_cpython/Include so it already gets copied into the build directory (or if we change how that works, it will at least still get the same treatment as all the other headers we have to make available).

@dagar dagar force-pushed the cmake-default branch 3 times, most recently from e312666 to 33297e7 Compare May 1, 2015 04:32
@dagar
Copy link
Contributor Author

dagar commented May 1, 2015

Moved SIZEOF_UNORDEREDMAP to pyconfig.h and renamed to pyconfig.h.in so there's no confusion.

@dagar
Copy link
Contributor Author

dagar commented May 7, 2015

I removed the original Makefile instructions because they'll no longer work.

@dagar
Copy link
Contributor Author

dagar commented May 7, 2015

Resolves issue #316.

@dagar
Copy link
Contributor Author

dagar commented May 7, 2015

Added Fedora dependencies, but ninja is actually ninja-build so we'll need to add a check to the Makefile.

@dagar dagar force-pushed the cmake-default branch 2 times, most recently from b21c11e to 161a0ef Compare May 7, 2015 04:10
@dagar dagar changed the title cmake change default build directory and configure script cmake change default build directory, configure script, Fedora support May 7, 2015
@dagar
Copy link
Contributor Author

dagar commented May 8, 2015

Added more header checks to pyconfig.h

@kmod
Copy link
Collaborator

kmod commented May 13, 2015

Sorry, can we split this up into different PRs? the stuff all looks good but I don't think we're ready to get rid of the makefile build system just yet, so we could get some of this in if we could split out a PR that doesn't remove the old system. for that PR, how about let's leave the makefile instructions in for now, and limit the pyconfig.h.in changes to the SIZEOF_UNORDEREDMAP define (which we can add makefile support for).

I think the things that make us keep looking at the makefile build system are using a custom gcc build (we've been experimenting with patching it and I don't think we figured out how to get cmake to use the patched version -- maybe @toshok can comment), and the valgrind mode (didn't seem to work, maybe @rntz can comment).

we're getting closer to not needing it though :)

@dagar
Copy link
Contributor Author

dagar commented May 19, 2015

make pyston_gcc already works with ~/pyston_deps/gcc-4.8.2-install/ and I made Valgrind work through the Makefile.

Do you want to just leave this open until we've resolved the remaining issues instead of hacking a size check into the Makefile?

Conflicts:
	from_cpython/Include/pyconfig.h.in
@dagar
Copy link
Contributor Author

dagar commented May 20, 2015

I merged with your pyconfig.h changes.

Apologies for the continued merges, the rebase was getting pretty hairy.

@dagar
Copy link
Contributor Author

dagar commented May 20, 2015

-closes #13
-closes #316
-closes #439

Conflicts:
	Makefile
	from_cpython/Include/dictobject.h
	from_cpython/Include/object.h
	from_cpython/Include/pyconfig.h.in
	src/CMakeLists.txt
@dagar
Copy link
Contributor Author

dagar commented Jun 3, 2015

Resolved the merge conflicts. Any outstanding issues here?

@dagar
Copy link
Contributor Author

dagar commented Jun 12, 2015

Resolved .travis.yml merge conflicts

@@ -1 +1 @@
Subproject commit 89f615a75fbc86d0dda71b407cb704077749c2cc
Subproject commit 7f14a66674f6d65c717199512e7ca705110c8da1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this change intentional? I wish github had better submodule support, but it looks like this is going back a few revisions so I assume it's a merge conflict.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how this slipped in. I'll rebase the bad merge so history is intact and take a final look at the full PR after all the merges.

@kmod
Copy link
Collaborator

kmod commented Jun 12, 2015

Looks great with some minor comments :) sorry again for the delay.

after we get this merged we can delete a few hundred lines from the makefile :)

@dagar dagar force-pushed the cmake-default branch 2 times, most recently from 2ca9a40 to 918d537 Compare June 12, 2015 03:29
-pyston build using the copied headers that include the generated pyconfig.h
@dagar
Copy link
Contributor Author

dagar commented Jun 12, 2015

@kmod - I fixed the libpypa submodule and set the pyston dependency to copy_stdlib instead of building the entire from_cpython.

kmod added a commit that referenced this pull request Jun 12, 2015
cmake change default build directory, configure script, Fedora support
@kmod kmod merged commit bafb715 into pyston:master Jun 12, 2015
@kmod
Copy link
Collaborator

kmod commented Jun 12, 2015

congrats! long live the new build system :)

@dagar dagar deleted the cmake-default branch June 13, 2015 01:02
@darnuria darnuria mentioned this pull request Mar 4, 2016
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

2 participants