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

cmake: produce civetweb.h, again #12008

Merged
merged 1 commit into from Nov 17, 2016
Merged

Conversation

mattbenjamin
Copy link
Contributor

The recent change to do this logic with file copy (and in src/rgw)
resolved the build problem, but now updates to the civetweb
submodule were not reflected in the build.

Move the copy into a custom target which will always source the
current submodule version at build time.

Signed-off-by: Matt Benjamin mbenjamin@redhat.com

Copy link
Member

@yehudasa yehudasa left a comment

Choose a reason for hiding this comment

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

lgtm, also tested

@ghost
Copy link

ghost commented Nov 16, 2016

https://jenkins.ceph.com/job/ceph-pull-requests/14144/console

[  0%] Building CXX object src/CMakeFiles/common_texttable_obj.dir/common/TextTable.cc.o
CMake Error: cmake version 2.8.12.2
Usage: /usr/bin/cmake -E [command] [arguments ...]
Available commands: 
  chdir dir cmd [args]...   - run command in a given directory
  compare_files file1 file2 - check if file1 is same as file2
  copy file destination     - copy file to destination (either file or directory)
  copy_directory source destination   - copy directory 'source' content to directory 'destination'
  copy_if_different in-file out-file  - copy file if input has changed
  echo [string]...          - displays arguments as text
  echo_append [string]...   - displays arguments as text but no new line
  environment               - display the current environment
  make_directory dir        - create a directory
  md5sum file1 [...]        - compute md5sum of files
  remove [-f] file1 file2 ... - remove the file(s), use -f to force it
  remove_directory dir      - remove a directory and its contents
  rename oldname newname    - rename a file or directory (on one volume)
  tar [cxt][vfz][cvfj] file.tar [file/dir1 file/dir2 ...]
                            - create or extract a tar or zip archive
  time command [args] ...   - run command and return elapsed time
  touch file                - touch a file.
  touch_nocreate file       - touch a file but do not create it.
Available on UNIX only:
  create_symlink old new    - create a symbolic link new -> old
make[3]: *** [src/rgw/CMakeFiles/civetweb_h] Error 1
make[2]: *** [src/rgw/CMakeFiles/civetweb_h.dir/all] Error 2

@mattbenjamin
Copy link
Contributor Author

(The jenkins report shows cmake w/the required commands, something wrong with quoting or formatting, or else the source file was not present [submodule]; will try to run it down)

@cbodley
Copy link
Contributor

cbodley commented Nov 16, 2016

testing this, i see that it forces a rebuild/link of radosgw every time you make, even if the civetweb.h is unchanged

you might be able to use cmake -E copy_if_different ... to work around this - otherwise i think add_custom_command() is what you want

@mattbenjamin
Copy link
Contributor Author

mattbenjamin commented Nov 16, 2016

@cbodley yeah, copy_if_different is more correct; I really prefer the target machinery, it's too bad cmake is (still?) slightly goofy in this area

The recent change to do this logic with file copy (and in src/rgw)
resolved the build problem, but now updates to the civetweb
submodule were not reflected in the build.

Move the copy into a custom target which will always source the
current submodule version at build time.

Avoid using the BYPRODUCTS option, as it is not supported in many
older cmake versions (e.g., Centos 7).

Signed-off-by: Matt Benjamin <mbenjamin@redhat.com>
@mattbenjamin
Copy link
Contributor Author

@yehudasa this should build on centos 7 now; I had tried to be clever and use the BYPRODUCTS option to make_custom_target--which most of our supported cmakes don't understand.

@yehudasa
Copy link
Member

@mattbenjamin lgtm, but I'm not sure I'm qualified to review this one

@mattbenjamin
Copy link
Contributor Author

@yehudasa This version built as expected, so whether it merges is I guess down to whether we want to resolve the problem of tracking civetweb submodule version.

@cbodley
Copy link
Contributor

cbodley commented Nov 17, 2016

retested and it works for me 👍

@mattbenjamin mattbenjamin merged commit 3f16167 into ceph:master Nov 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants