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

One build directory per branch/arch. #4286

Merged
merged 1 commit into from
Sep 14, 2018

Conversation

cslashm
Copy link
Contributor

@cslashm cslashm commented Aug 20, 2018

One build directory per branch/arch.

This proposal allows to perform multiple compilation from different branch/arch in
separate directories.
Example:

    build
    ├── GNU_Linux
    │   ├── multi-compilation
    │   │   └── release
    │   └── NanoS-USBHID
    │       └── release
    └── Msys
        └── NanoS-USBHID
            └── release

Edit 1:

Try to handle special char as : / \ .

--data-dir in unit test not yet tested

Edit 2:

donot use param for uname. -o is not supported by MacOS.

Makefile Outdated
@@ -26,105 +26,109 @@
# STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
# THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.


subbuilddir:=$(shell uname -o|sed -e 's|/|_|g')/$(shell git branch |grep '\* ' |cut -f2 -d' ')
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think windows might not like that. Can someone with mingw test ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I test it under win7. The sample tree is mine

Makefile Outdated
mkdir -p build/debug
cd build/debug && cmake -D CMAKE_BUILD_TYPE=Debug ../..
mkdir -p build/$(subbuilddir)/debug
cd build/$(subbuilddir)/debug && cmake -D CMAKE_BUILD_TYPE=Debug ../../../..
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you have a branch name in this, then you need to start quoting.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And replacing / with something else (I do have a branch with a / in so it's not pedantic :P)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ooops, never thought about / in a branch name :D

Ok I will add some quote here and there and test with OMG branch name.(I just mod the Makefile for my own purpose and thought it could be useful for other with too slow machine :p)

@moneromooo-monero
Copy link
Collaborator

Also, check whether unit tests --data-dir need fixing to add two ../

@cslashm cslashm force-pushed the multi-compilation branch 3 times, most recently from dd46e3d to 285e6c7 Compare August 20, 2018 21:23
@stoffu
Copy link
Contributor

stoffu commented Aug 21, 2018

@moneroexamples
Copy link
Contributor

moneroexamples commented Aug 21, 2018

If these changes are beneficial for monero, then moneroexamples can be adopted to this change. However, I don't know how this change will propagate when checking out older monero releases, which I often do?

For example if I make new branch based on v12.3.0 (git checkout -b last_release v0.12.3.0) and compile this new branch to have v0.12.3 monero libraries, this new build directory scheme obviously will not be used. And this complicate things, at least till the new scheme will be made official with next monero release.

@cslashm
Copy link
Contributor Author

cslashm commented Aug 21, 2018

Is it an acceptable solution (hack?) to copy in the old scheme the last binaries build?

@sammy007
Copy link

sammy007 commented Aug 22, 2018

@stoffu I only need cmake -DBUILD_SHARED_LIBS=1 . to be there due to CGO https://github.com/sammy007/monero-stratum#linux. If there is easy way to pass path to subdirectory via CMake it should not be a problem.

@moneromooo-monero
Copy link
Collaborator

I've tried it, and I realised it does spam the build tree a lot. If you have lots of branches, you end up having to manually clean all the time.
I've made it optionally off: http://paste.debian.net/hidden/04b09457/

@luigi1111
Copy link
Collaborator

@cslashm please rebase.

@cslashm
Copy link
Contributor Author

cslashm commented Sep 12, 2018

@moneromooo-monero nice idea, I will mod

@luigi1111, I merge the proposition from moneromooo and rebase that. I hope tomorrow, the day after else.

@cslashm
Copy link
Contributor Author

cslashm commented Sep 13, 2018

  • Off option added
  • clean-all rule added to remove ALL builds
  • master rebased

@stoffu
Copy link
Contributor

stoffu commented Sep 13, 2018

If I build with HEAD detached, space and parenthesis characters in the build directory name cause syntax errors:

/bin/sh: -c: line 0: syntax error near unexpected token `('
/bin/sh: -c: line 0: `echo "WARNING: Back-up your wallet if it exists within ./"build/Darwin/(HEAD detached at upstream_pr_4286)"!" ;         read -r -p "This will destroy the build directory, continue (y/N)?: " CONTINUE; [ $CONTINUE = "y" ] || [ $CONTINUE = "Y" ] || (echo "Exiting."; exit 1;)'
make: *** [clean] Error 2

Fix:

@@ -28,7 +28,7 @@
 
 ANDROID_STANDALONE_TOOLCHAIN_PATH ?= /usr/local/toolchain
 
-subbuilddir:=$(shell echo  `uname | sed -e 's|[:/\\]|_|g'`/`git branch | grep '\* ' | cut -f2- -d' '| sed -e 's|[:/\\]|_|g'`)
+subbuilddir:=$(shell echo  `uname | sed -e 's|[:/\\ \(\)]|_|g'`/`git branch | grep '\* ' | cut -f2- -d' '| sed -e 's|[:/\\ \(\)]|_|g'`)
 ifeq ($(USE_SINGLE_BUILDDIR),)
   builddir="build/$(subbuilddir)"
 else

@stoffu
Copy link
Contributor

stoffu commented Sep 13, 2018

As for 3rd party software:

@moneroexamples
The only requirement is to pass the USE_SINGLE_BUILDDIR switch to the command:

USE_SINGLE_BUILDDIR=1 make

@sammy007
Ah, your software assumes cmake to be called directly from the command line. Then I think this patch has no effect for it.

@cslashm cslashm force-pushed the multi-compilation branch 2 times, most recently from f62a5e7 to 02d8bfc Compare September 13, 2018 09:35
@cslashm
Copy link
Contributor Author

cslashm commented Sep 13, 2018

@stoffu add both: your last proposition-edit plus a modification of your first one in clean rule

Makefile Outdated
builddir="build/$(subbuilddir)"
else
builddir="build"
endif
Copy link
Contributor

@MoroccanMalinois MoroccanMalinois Sep 13, 2018

Choose a reason for hiding this comment

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

This introduces distinct levels of subdirectories, but the number of ../ is fixed. So for me, it doesn't work when using USE_SINGLE_BUILDDIR=1

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I used -C all the time...
https://paste.debian.net/hidden/3254c3ef/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My fault too I would have to test it...

This proposal allows to perform multiple compilation from different branch/arch in
separate directories.
Example:

    build
    ├── GNU_Linux
    │   ├── multi-compilation
    │   │   └── release
    │   └── NanoS-USBHID
    │       └── release
    └── Msys
        └── NanoS-USBHID
            └── release

Edit 1:

Try to handle special char as : / \ .

--data-dir in unit test not yet tested

Edit 2:

donot use param for uname. -o is not supported by MacOS.
Copy link
Contributor

@fluffypony fluffypony left a comment

Choose a reason for hiding this comment

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

Reviewed

@fluffypony fluffypony merged commit b4679f3 into monero-project:master Sep 14, 2018
fluffypony added a commit that referenced this pull request Sep 14, 2018
b4679f3 One build directory per branch/arch. (cslashm)
@cslashm cslashm deleted the multi-compilation branch March 21, 2019 15:15
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.

8 participants