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

Fix parallel build #280

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

Fix parallel build #280

wants to merge 3 commits into from

Conversation

dermoth
Copy link

@dermoth dermoth commented Aug 3, 2017

Should we also figure out 2nd stage issue?

@dermoth dermoth changed the title Fix parallel build (at least first stage) Fix parallel build Aug 6, 2017
@goatpig
Copy link
Owner

goatpig commented Oct 1, 2017

Should we also figure out 2nd stage issue?

you mean -march=native? Definitely

@dermoth
Copy link
Author

dermoth commented Oct 1, 2017

I meant the 2nd commit, that one fixes the 2nd stage. The problem was that phony target which forced a recompile CppBlockUtils_wrap.cxx on which all code code depends (so all code would recompile); instead I included the proper deps to CppBlockUtils_wrap.cxx so it will recompiles only if the headers change.

The --march issue, I don't recall exactly... I build my AspireOne i386 binaries on a VM where the CPU features are forces to an older model. I'll have to look again if something needs fixing, but IMHO it's a different issue, this PR could be merged.

@goatpig
Copy link
Owner

goatpig commented Oct 1, 2017

I see. Sounds good. The march issue is that currently I can't build on my CPU without at least -march=native, which is miserable. I'm trying to do one failsafe build with no special instruction sets.

At any rate, could you rebase this to the current state of testing please?

@droark
Copy link

droark commented Jan 14, 2018

@dermoth - Did you ever do a rebase? Doesn't look like it.

@dermoth
Copy link
Author

dermoth commented Jan 14, 2018

Aww my bad... will try to get back at it shortly!

@dermoth dermoth changed the base branch from testing to dev February 13, 2018 11:24
@dermoth
Copy link
Author

dermoth commented Feb 13, 2018

I also rebased to dev as it appears to be more current that testing... There see to be one issue though... one header file appears to have been removed from cppForSwig and fails dependency checks... Since I took the header list form existing Makefile definitions it's not really "my" bug but still breaks the build on latest code.

I will not only remove it but also review changes to make sure all header files are also included there.

Regards...

@dermoth
Copy link
Author

dermoth commented Feb 13, 2018

It was actually a typo... I inserted it before my commit to prevent breakages.

I tested with the latest code that:

  1. Rebuild do not force recompiles of CppBlockUtils_wrap.cxx
  2. Debian build uses all available CPUs by default

Regards,

@droark
Copy link

droark commented Feb 13, 2018

The dev branch is basically the 0.97 branch. It's actually broken at the moment, at least on Linux and macOS. Also, I have some PRs out there that will significantly alter the build system in 0.97. Here's what I would recommend.

  • Test the PR on testing. If it works, just copy over your changes.
  • Ping me so that I can review your changes and let you know if they're compatible with the forthcoming changes.
  • Wait for my changes to be accepted, which will hopefully be soon.
  • Comment out the new CPP files and try compiling. I think Armory will compile at that point? If not, we'll take it from there. A second commit might be necessary later depending on this and that.

Thanks for keeping this alive. Much appreciated.

EDIT: If you want to see what's in the pipeline, look here. It's also against dev but I ran it against testing in order to ensure that it works.

@dermoth
Copy link
Author

dermoth commented Feb 15, 2018

Just to clarify:

  1. The header file was not missing, there was a typo in the file name in Makefile.am. If you don't merge this PR at least cherry-pick the commit that fixes the typo. The commits are also split up so it should be easy to cherry-pick only what's needed in testing. I could open a separate PR for testing if you like...

  2. Armory built fine on dev from Debian 9 (Stretch) and default gcc (Debian 6.3.0-18) 6.3.0 20170516. I haven't tested the 64bit build nor tried running it though.

@dermoth
Copy link
Author

dermoth commented Jan 3, 2019

Looking back into this, the INCLUDE_FILES variable have been removed altogether, hence the conflicts (at least it fixes the typo... it's all gone!)

CppBlockUtils_wrap.cxx is still a phony target so it's going to be rebuilt over and over (swig itself isn't that long to run but IIRC it then trigger a cascade of dependencies to be rebuilt as well).

Any interest in fixing that? FWIW we can find swig dependencies (to be added to Makefile.am) using the -MM switch.

The last commit could be added standalone, but my recollection is that the commit alone didn't speed up that much because of all the time spend building and rebuilding swig.

If there is still interest in this please let me know which branch I should rebase the fix against.

Regards

@droark
Copy link

droark commented Jan 3, 2019

Hey. Thanks for hanging in there. The Makefile system has undergone quite a few changes in the last year or so. Some more changes will be merged into dev soon-ish. Once the changes are merged in, is there any chance that you can rebase? If you can also fix the SWIG stuff getting rebuilt every time, I'd be happy. There's a partial workaround now (disable the GUI) but if there's an efficient way to know when the SWIG stuff should be rebuilt, that would help a lot.

@goatpig
Copy link
Owner

goatpig commented Jan 3, 2019

A Makefile cleanup is welcomed, but consider that I'm looking to replace swig with CFFI for the next major version, so changes around swig will most likely never make it into a release at this point.

@dermoth
Copy link
Author

dermoth commented Jan 3, 2019

Well unless you care about changing system libraries between tests then I would only add a couple header files as additional dependencies of CppBlockUtils_wrap.cxx, those being returned by swig itself, and remove the .PHONY target. It's a very simple change.

@goatpig
Copy link
Owner

goatpig commented Jan 11, 2019

Path is clear for any amount of PRs in the dev branch right now, feel free to submit.

@dermoth
Copy link
Author

dermoth commented Jul 19, 2019

I need to dive into this again... I haven't needed to compile Armory for a while now. The fix was simple though - CppBlockUtils_wrap.cxx needed to depend on the swig auto-generated glue headers, and removed from .PHONY. As I said the only risk is if the libraries are swapped between builds, so if any external deps are upgraded you should start off a clean tree.

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.

3 participants