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

Add Bazel as build engine #5

Closed
wants to merge 2 commits into from
Closed

Conversation

mikefaille
Copy link
Contributor

No description provided.

@mikefaille
Copy link
Contributor Author

@AXDOOMER

@@ -20,7 +20,7 @@
#include "vecmath.h" /* Float3, ComputeNormal */
#include "things.h" /* Player, Plane */
#include "cache.h" /* Cache */
#include "physics.h" /* AdjustPlayerToFloor */
// #include "physics.h" /* AdjustPlayerToFloor */
Copy link
Owner

Choose a reason for hiding this comment

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

Do not make changes to my source code.

@@ -149,7 +149,7 @@ void Level::LoadLevel(const string& LevelName, unsigned int numOfPlayers)
if (EndsWith(LevelName, ".obj"))
{
LoadObj(LevelName);
AdjustPlayerToFloor(play, this);
// AdjustPlayerToFloor(play, this);
Copy link
Owner

Choose a reason for hiding this comment

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

Do not make changes to my source code.

Copy link
Owner

@AXDOOMER AXDOOMER left a comment

Choose a reason for hiding this comment

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

Do not make changes to my source code.

@mikefaille
Copy link
Contributor Author

mikefaille commented Oct 29, 2018

The commit is still not ready and it's an experimentation just for fun. Actually, there is dependency loop needed to be solved. One way to do this is by extracting code common from Physics.h or Level.h and put this code on new file. Still, fixing a dependency loop is the right path to use Bazel and leverage safe build cache with build parallelism.

The constraint here to fix the loop, is the wanted cohesion the over original design.

Moreover, this dependency loop don't cause trouble if we don't mind about faster build using build cache because compilation can be done using run.sh the root of the project using a guarantied sequence; by this way, the loop have no effect on build reproducibility.

@mikefaille mikefaille closed this Oct 29, 2018
@mikefaille
Copy link
Contributor Author

mikefaille commented Oct 29, 2018

́ M'a tout changer ça dans pas long en plus. Ton PR va être outdated

Raison du PR closed ;-) --^

@AXDOOMER
Copy link
Owner

AXDOOMER commented Oct 30, 2018

Good catch about the dependency loop, but I don't think much can be done about it without a lot of work.

The "physics" functions need to know about the level and the "level" code must be able to call these functions. If Bazel can't handle this, then that seems lame, but it's a good thing.

Indeed your code change revealed that AdjustPlayerToFloor is the only function from "physics" being called in the "level" code. Originally, I called it in main.cpp, but this file was handling too many different things. It's right though that it's not the level's job to call this function. I should fix this. Code refactoring will be required.

My source code files keep changing, that's why my makefile and run.sh don't care about the sequence and find files dynamically. The makefile works great if you don't want to recompile everything every time.

@mikefaille
Copy link
Contributor Author

mikefaille commented Oct 30, 2018

All agreed and great feedback. Just one thing, file glob from run.sh add dependencies sequentially by alphanumerical order and this script is actually the only way to get reproducible builds. Still, in the current state it's possible to recreate corrupt build cache with current makefile if we shuffle by times changes+builds order between part of level.cpp and physics.cpp. Then, using the same source code we can get different binaries with different bugs. So, run.sh do job wanted without issue for sure until you refactor your stuff like Héphaïstos.

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