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

Mixing of source code and build results during the build process #350

Closed
WimK opened this issue Jun 30, 2016 · 13 comments
Closed

Mixing of source code and build results during the build process #350

WimK opened this issue Jun 30, 2016 · 13 comments
Assignees
Milestone

Comments

@WimK
Copy link

WimK commented Jun 30, 2016

While creating source code packages I ran into a problem that I believe makes it unnecessarily difficult to create packages in a safe way. This mainly has to do with the way Conan currently sets up the build directory during a build (for instance after doing conan install .. --build)

What Conan currently does is to copy the contents of the package's "source" directory (in the .conan/data location) into a configuration specific build directory. After the copy has been performed the "build" method of the conanfile.py is invoked, with the build directory as the default working location for the "build" method. In other words the build directory is the same as the source directory.

In many cases, especially when packaging from a source project, this means that the build directory will have both the full source as well as the build results mixed in the same directory. This is also the case, for instance, in the "hello" library example from the "Packaging from the source project"-chapter of the manual. The build results (i.e. directories lib, bin and all the files from the CMake process) will be mixed with hello.cpp and hello.h, all in the same directory.

For the hello example this does not cause any problems. However, in other cases this mixing can have undesired consequences. For instance when certain files are generated during the build process and overwrite original source or project files, or if the build process runs certain code depending on the presence or absence of files in the build directory. For this reason the hello example is not safe for general usage.

Of course, having conanfile.py allows you to make any modification to the build process to prevent problems (e.g. move the build directory source code to another location before the build starts). This requires a thorough understand of Conan and the way packages are being build. While at the same time preventing, or at least make it much more risky, one of the most common use cases (i.e. having a simply conanfile.py in the root of your library project and running only basic cmake commands in build()).

My suggestion would be to copy the source code during the build step to another directory that is separate from the actual build location and make the location of this source directory available in the build() method so it can be used in the build step.

Edit: An alternative suggestion would be to make the copy of the source to build step optional by setting a flag in conanfile.py (e.g. copy_source_for_build=False) and to make the location of the base source directory available in the build() method.

@memsharded
Copy link
Member

What is you concern about safety? That sources could be packaged to a binary package, and you don't want to distribute the source code?. I see the issue, and it could be a good idea if possible.
But the problem might be that it is impossible for other build systems than cmake, which allows out-of-source. In the general case, the build is in-source, and there are many conan packages that do not use cmake, and thus require the in-source build.
So that is why it cannot be separated by conan, but the user can do it in their conanfiles. Does this make sense?

@WimK
Copy link
Author

WimK commented Jun 30, 2016

I meant 'safe' as in without worrying about side effects, not in the context of security.

Most CMake projects will not be designed for in-source builds, many CMake projects will depend on the build directory being empty or at least not have the entire source there, and some CMake projects even flat out refuse to build when a trying to perform an in-source build (e.g. set(CMAKE_DISABLE_IN_SOURCE_BUILD ON)).

If you want to package such projects with a conanfile.py you have to perform quite a bit of trickery to make it work. I have tried it by making a _build subdirectory within the "build/" directory that contains the source code. The problem in this case is that Conan will place the conanbuildinfo.cmake file in the build/\<hash\> directory and cannot be found with my default CMakeLists.txt that does include(${CMAKE_BINARY_DIR}/conanbuildinfo.cmake) (which works when I build my project normally).

Having a flag available in conanfile.py that allows you to specify whether the source code has to be copied in the build directory would solve this problem and still allow for in-source builds. Having an extra variable available to specify the location of the source directory (e.g. self.source_directory) would make it even transparent whether you are building in-source or not.

So for instance with the Hello example from the documentation:

class HelloConan(ConanFile):
    name = "Hello"
    version = "0.1"
    settings = "os", "compiler", "build_type", "arch"
    exports = "*"
    copy_source_to_build_dir = False 

    def build(self):
        cmake = CMake(self.settings)
        self.run('cmake %s %s' % (self.source_directory, cmake.command_line))
        self.run("cmake --build . %s" % cmake.build_config)
...

If copy_source_to_build_dir is set to False the source code will not be copied and self.source_directory would be set to the external source code directory, while if it is set to True the source will be copied and the self.source_directory variable will be set to the build directory containing the source code. If copy_source_to_build_dir would default to True it would not even break any current packages.

@lasote
Copy link
Contributor

lasote commented Jun 30, 2016

I agree about preparing a "build" subfolder can be tricky, and also in the include of conanbuildinfo.cmake in the CMakeLists.txt, this is a real example:

if(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/build/conanbuildinfo.cmake) #Clion, with conanbuildinfo.cmake in root
    include(${CMAKE_CURRENT_SOURCE_DIR}/build/conanbuildinfo.cmake)
elseif(EXISTS ${CMAKE_BINARY_DIR}/../conanbuildinfo.cmake) # As a conan package
    include(${CMAKE_BINARY_DIR}/../conanbuildinfo.cmake)
else()
    include(${CMAKE_BINARY_DIR}/conanbuildinfo.cmake) # Not CLion
endif()

It seems we could do it better.
But I'm not sure at all about the approach, I would like something more explicit, like a helper that prepares a build folder, move the conanbuildinfo.cmake to it and change the curdir.

Just an idea to discuss:

class HelloConan(ConanFile):
    name = "Hello"
    version = "0.1"
    settings = "os", "compiler", "build_type", "arch"
    exports = "*"

    def build(self):
        cmake = CMake(self.settings)
        with outsource_folder("build") as folder:
            self.run('cmake %s %s' % (self.conanfile_directory, cmake.command_line))
            self.run("cmake --build . %s" % cmake.build_config)

@memsharded
Copy link
Member

I suggest giving the responsibility to the CMake helper. As far as I know, it would be the only one able to do out-of-source

@WimK
Copy link
Author

WimK commented Jul 1, 2016

I suggest giving the responsibility to the CMake helper. As far as I know, it would be the only one able to do out-of-source

CMake is a generator system, producing Make, Visual Studio, Ninja, etc build files. So these all support out of source builds if CMake is able to produce them. Admittedly, doing out of source builds with Make is not particularly common.
qmake also supports out of source builds in the form of shadow builds.

I think the solution by @lasote would probably works. Although personally I believe it would be nicer if copying the source code to the build folder is an optional step - something that can be prevented if not required. Building a library like LLVM/Clang would now result in several hundred MB being copied, unnecessarily, for every different different build type. Being able to say "don't bother copying, I'm building out of source" would really help.

@memsharded
Copy link
Member

I think I misunderstood the issue, I was not thinking about taking the sources from the source folder, just about separating the copied sources in the build folder from the actual build folder.
Taking the sources from the original source folder actually sounds as reasonable, and the optimization is real (I'd say even a better reason). But this is clearly a tradeoff. I don't dislike the feature, but I still have some concerns:

  • It makes the conan codebase more complex. It is not only about not copying the sources for building, but the package() method would be a mess, trying to copy headers from the source folder and libraries from the build folder. Please note that the package() method uses patterns to scan the package folder, having to do so also in the other folder can be tricky and error prone.
  • It will introduce errors when misused and changing the source folder from the build() method (building in-source).

I understand that the copy takes time and space, but it is usually just a fraction compared with the build times or the disk space the builds use. Such space is temporary, can be removed easily (conan remove * -b). So I would be more inclined to implement this when it actually becomes a real pain. Are you already packaging LLVM? Can you give us some numbers? The moment you or someone else is actually packaging LLVM (I think some other user did), and actual copy times can be measured and compared, this feature will get higher priority. You know, avoid premature optimization :)

@bjmacdonald
Copy link

bjmacdonald commented Apr 12, 2017

I've recently started using conan for cmake builds and had a look at this. It turns out to be very straightforward to do, only a few lines in conans/client/{installer.py,packager.py}. I added a conanfile flag "no_copy_source" to suppress the copy and an additional copy_source instance of FileCopier to facilitate packaging.

My use case is probably reasonably popular - I use cmake for all builds out of source and the install target to push the files to the package. I also don't use anything in the conanbuildinfo.cmake script except the module path as I use cmake exported targets in the package, hence I don't bother setting package_info(). An example conanfile.py:

import os
from conans import ConanFile, CMake, tools


class LibreSSLConan(ConanFile):
    name = "libressl"
    version = "2.5.2"

    url = "csl-conan-recipes.git"
    license = "MIT"
    generators = "cmake"
    build_policy = "missing"

    zip_folder = name + "-" + version
    zip_file = zip_folder + ".tar.gz"

    description = "LibreSSL, OpenSSL compatible library."

    settings = "os", "arch", "compiler" # , "build_type"

    exports_sources = ["CMakeLists.txt", zip_file, "cmake/*"]
    exports = ["FindOpenSSL.cmake", "FindLibreSSL.cmake"]
    no_copy_source = True

    def source(self):
        tools.unzip(self.zip_file)
        os.unlink(self.zip_file)
        for (dp, dns, fns) in os.walk("cmake"):
            op = os.path.join(self.zip_folder, os.path.relpath(dp, "cmake"))
            for fn in fns:
                os.replace(os.path.join(dp,fn), os.path.join(op,fn))

    def build(self):
        # use Ninja for speed
        cmake = CMake(self, generator="Ninja")

        # absolute path to actual source folder
        cmake_source_dir = os.path.join(self.source_folder, self.zip_folder)

        # cmake defines as a dict
        cmake_defs = {"CMAKE_INSTALL_PREFIX" : self.package_folder}

        # build both variants
        if cmake.is_multi_configuration:
            cmake.configure(source_dir=cmake_source_dir, build_dir="Debug_Release", defs=cmake_defs)
            for config in ("Debug", "Release"):
                cmake.build(args=["--config",config], target="install")
        else:
            for config in ("Debug", "Release"):
                cmake_defs["CMAKE_BUILD_TYPE"] = config
                cmake.configure(source_dir=cmake_source_dir, build_dir=config, defs=cmake_defs)
                cmake.build(target="install")

    def package(self):
        # copy find package
        self.copy_source("FindOpenSSL.cmake")
        self.copy_source("FindLibreSSL.cmake")

    def package_info(self):
        pass


# Sneaky self-registration script!
if __name__ == "__main__":
    from conans.conan import main
    main("export capitis/stable".split(" "))

Anyway, here are the diffs. I have never submitted a pull request but if there is any interest please let me know and I'll try to figure it out:

  conans/client/installer.py | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/conans/client/installer.py b/conans/client/installer.py
index a678ab7..e7a9c2c 100644
--- a/conans/client/installer.py
+++ b/conans/client/installer.py
@@ -448,9 +448,14 @@ Or read "http://docs.conan.io/en/latest/faq/troubleshooting.html#error-missing-p
                     output.warn("Filename too long, file excluded: %s" % dest_path)
             return filtered_files
 
-        shutil.copytree(src_folder, build_folder, symlinks=True, ignore=check_max_path_len)
-        logger.debug("Copied to %s" % build_folder)
-        logger.debug("Files copied %s" % os.listdir(build_folder))
+        if getattr(conan_file, 'no_copy_source', False):
+            # conan_file.source_folder = os.path.normpath(os.path.join(build_folder, '../../source'))
+            conan_file.source_folder = src_folder
+            os.makedirs(build_folder)
+        else:
+            shutil.copytree(src_folder, build_folder, symlinks=True, ignore=check_max_path_len)
+            logger.debug("Copied to %s" % build_folder)
+            logger.debug("Files copied %s" % os.listdir(build_folder))
         os.chdir(build_folder)
         conan_file._conanfile_directory = build_folder
         # Read generators from conanfile and generate the needed files
 conans/client/packager.py | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/conans/client/packager.py b/conans/client/packager.py
index 8189c36..311ba9a 100644
--- a/conans/client/packager.py
+++ b/conans/client/packager.py
@@ -31,8 +31,26 @@ def create_package(conanfile, build_folder, package_folder, output, local=False)
     conanfile.copy_libs = wrap(DEFAULT_LIB)
     conanfile.copy_bins = wrap(DEFAULT_BIN)
     conanfile.copy_res = wrap(DEFAULT_RES)
+
+    if getattr(conanfile, 'no_copy_source', False):
+        conanfile.source_folder = os.path.normpath(os.path.join(build_folder,'../../source'))
+        conanfile.copy_source = FileCopier(conanfile.source_folder, package_folder)
+
+        def wrap_src(dst_folder):
+            def new_method(pattern, src=""):
+                conanfile.copy_source(pattern, dst_folder, src)
+            return new_method
+
+        conanfile.copy_source_headers = wrap_src(DEFAULT_INCLUDE)
+        conanfile.copy_source_libs = wrap_src(DEFAULT_LIB)
+        conanfile.copy_source_bins = wrap_src(DEFAULT_BIN)
+        conanfile.copy_source_res = wrap_src(DEFAULT_RES)
+
     try:
         conanfile.package()
+        if getattr(conanfile, 'no_copy_source', False):
+            package_source_output = ScopedOutput("%s package_source()" % output.scope, output)
+            conanfile.copy_source.report(package_source_output, warn=True)
         package_output = ScopedOutput("%s package()" % output.scope, output)
         conanfile.copy.report(package_output, warn=True)
     except Exception as e:

@memsharded
Copy link
Member

Thanks very much for contributing your implementation. The main issue is that it is adding a bit of complexity, and could have special corner-cases and unexpected behavior (imagine someone copy & paste a recipe with no_copy_sources, then using the typical in-source .configure & make). But we will review it and consider it for next 0.23. If it is accepted, then yes, I would encourage to submit a Pull request, so you get credit for it. Cheers!

@memsharded memsharded added this to the 0.23 milestone Apr 14, 2017
@memsharded
Copy link
Member

memsharded commented Apr 30, 2017

Hi @bjmacdonald,

I am starting to review, and liking the idea. It results that defining a conanfile.source_folder might also help to smooth the local edition workflows (#1171), in which it is useful to be able to point the recipe to the source folder that your IDE has opened.

@bjmacdonald
Copy link

I had to make a small change to check if the build folder already exists in installer.py, please see below for diff. Otherwise it seems to work fine with the latest develop branch from the repository. I also added the members to conan_file.py but still get a pylint warning about copy_source not being callable...

I will have to look closer at the local edition workflows - one thing that has been nagging me is the capability to easily configure a conan local build if I need to step into 3rd party libraries with the debugger.

 conans/client/installer.py | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/conans/client/installer.py b/conans/client/installer.py
index 8685331..8163127 100644
--- a/conans/client/installer.py
+++ b/conans/client/installer.py
@@ -452,9 +452,15 @@ Or read "http://docs.conan.io/en/latest/faq/troubleshooting.html#error-missing-p
                     output.warn("Filename too long, file excluded: %s" % dest_path)
             return filtered_files
 
-        shutil.copytree(src_folder, build_folder, symlinks=True, ignore=check_max_path_len)
-        logger.debug("Copied to %s" % build_folder)
-        logger.debug("Files copied %s" % os.listdir(build_folder))
+        if getattr(conan_file, 'no_copy_source', False):
+            # conan_file.source_folder = os.path.normpath(os.path.join(build_folder, '../../source'))
+            conan_file.source_folder = src_folder
+            if not os.path.exists(build_folder):
+                os.makedirs(build_folder)
+        else:
+            shutil.copytree(src_folder, build_folder, symlinks=True, ignore=check_max_path_len)
+            logger.debug("Copied to %s" % build_folder)
+            logger.debug("Files copied %s" % os.listdir(build_folder))
         os.chdir(build_folder)
         conan_file._conanfile_directory = build_folder
         # Read generators from conanfile and generate the needed files
 conans/model/conan_file.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/conans/model/conan_file.py b/conans/model/conan_file.py
index dc46a08..66d9267 100644
--- a/conans/model/conan_file.py
+++ b/conans/model/conan_file.py
@@ -109,6 +109,7 @@ class ConanFile(object):
         self.deps_env_info = DepsEnvInfo()
 
         self.copy = None  # initialized at runtime
+        self.copy_source = None  # initialized at runtime
 
         # an output stream (writeln, info, warn error)
         self.output = output
@@ -117,6 +118,7 @@ class ConanFile(object):
 
         self._conanfile_directory = conanfile_directory
         self.package_folder = None  # Assigned at runtime
+        self.source_folder = None  # Assigned at runtime
         self._scope = None
 
         # user specified env variables

@memsharded
Copy link
Member

Cool thank you! Will take it into account, and keep updated here.

@memsharded
Copy link
Member

I have done some initial work on this in #1259.

I needed to go with a different approach, to account for an homogeneous model:

  • The no_copy_source is used and means the same.
  • Defined source_folder and build_folder. source_folder will point in conan local cache to the build folder if no_copy_source=False or not defined.
  • The package() method, operates with the same self.copy method, not creating a new one. It will do 2 copies, one from the source folder if required.
  • The CMake helper now uses the source_folder and build_folder, much cleaner and explicit.

In this way, the local edition workflows are fully supported. Note than when a package is under development in user space, it is necessary to support having up to 3 folders coming from 3 repos: the package recipe, the package source code and the temporary build folder.

As another advantage, it might allow users to adopt it, just setting no_copy_source attribute (given that they have an updated CMake syntax (cmake.configure() helper), without having to modify the package() method.

Still need much more testing and completing the joint proposal.

@memsharded memsharded self-assigned this May 5, 2017
@lasote lasote added the fixed label May 17, 2017
@memsharded
Copy link
Member

Hi @WimK, the no_copy_source attribute has been implemented in 0.23 to allow that. Please check: http://conanio.readthedocs.io/en/latest/reference/conanfile/attributes.html#no-copy-source

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

No branches or pull requests

5 participants