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

Improve build system #160

Merged
merged 23 commits into from
Apr 10, 2017
Merged

Improve build system #160

merged 23 commits into from
Apr 10, 2017

Conversation

CrossVR
Copy link
Contributor

@CrossVR CrossVR commented Jan 27, 2017

This is a PR to improve the build process:

  • Automatically build luajit
  • Don't build the unmaintained editor projects
  • Add every external project as a submodule

The goal is to be able to do a simple git clone --recursive to clone XRay 1.6 and all dependencies to allow the developer to simply hit Build Solution without errors.

@jakebriggs
Copy link

I still had to compile luajit from the command line before everything compiled - but once I did that, it compiled fine.

@andrew-boyarshin
Copy link
Contributor

@Armada651 why have you decided to switch to static build of Crypto++? FIPS conformance is not required by OpenXRay engine (we do not handle money or sensitive user data), plus both before and after this commit we cannot claim to be FIPS conformant.

@CrossVR
Copy link
Contributor Author

CrossVR commented Mar 10, 2017

@andrew-boyarshin That's exactly the reason why I switched to a static build. Crypto++ automatically does some FIPS validation tests if it's a DLL build, most notably it checks the hash of the DLL which fails because I didn't include the hash in the output DLL.

Also it is only used in a very small part of the engine, namely checking if certain resource files are "pure" during online gameplay.

@andrew-boyarshin
Copy link
Contributor

@Armada651 oh, I see. I was not aware of runtime validation tests, and their website was not informative enough to tell me that fact.

@andrew-boyarshin
Copy link
Contributor

Tried out and confirmed to be working. Looks Good To Me.

@andrew-boyarshin
Copy link
Contributor

andrew-boyarshin commented Mar 18, 2017

What has changed so that you've force-pushed? Just rebasing on latest commit?

P.S. When it is going to be reviewed?

@Xottab-DUTY
Copy link
Member

@andrew-boyarshin he just rebased his branch to the latest commits after merging my PRs.
P.S. So, the branch isn't updated to the latest merged precompiled shaders PR.

@CrossVR
Copy link
Contributor Author

CrossVR commented Mar 19, 2017 via email

Copy link
Contributor

@andrew-boyarshin andrew-boyarshin left a comment

Choose a reason for hiding this comment

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

Debug:
Works fine, no flaws, although I also had to apply xrSound CDB library linking fix from another PR for Debug code to compile.

Release:
I had some troubles. zlib in Release mode requires contrib asm linked in. After building these asm files (manually, does not quite match the task of the PR) and successfully linking it with the rest of the zlib I've encountered issue with BugTrap not being able to link to it due to SAFESEH enabled ("no pure asm" requirement).
This should be resolved in order to be able to merge it. Of course removing ASMV and ASMINF preprocessor flags helped to resolve the issue. And taking into consideration asm code is considered experimental in zlib - I think that's the way to go.

@CrossVR
Copy link
Contributor Author

CrossVR commented Mar 30, 2017

@andrew-boyarshin Issues are fixed, I opted to disable the assembler code, because it doesn't seem like it wasn't used before either.

@andrew-boyarshin
Copy link
Contributor

@Armada651 will check this out and update my review.

@CrossVR
Copy link
Contributor Author

CrossVR commented Apr 10, 2017

Since no one seems to disagree with these changes following the review I'm going to merge them tomorrow. Let me know if anyone has time to do a full review.

@andrew-boyarshin
Copy link
Contributor

In fact, I've reviewed your modifications and they look good to me. I just had little time to do the thorough testing in the game.

@CrossVR
Copy link
Contributor Author

CrossVR commented Apr 10, 2017

The only real risk to this PR is the fact that the change to Crypto++ is mostly untested (only some sanity tests were done). The only consequence of that would be that it would break "pure" multiplayer. I don't think multiplayer is a priority anyway, so if it turns out we broke that we can fix it later.

@CrossVR CrossVR merged commit 54f4f68 into OpenXRay:dev Apr 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants