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

Removed optional behavior (shopt) from scripts #280

Merged
merged 3 commits into from
Mar 27, 2019

Conversation

thoasm
Copy link
Member

@thoasm thoasm commented Mar 24, 2019

Removed the need for the following requested features in the scripts dev_tools/scripts/update_ginkgo_header.sh and dev_tools/scripts/add_license.sh:

  • extglob (**) by using find instead
  • globstar with appropriate arguments for find

Additionally, made sure that commands that are used (like find, sort, ...) are present and do not return an error.
I am not 100% sure if this check is necessary (or if I should also include cat, rm, mv and so on), but I don't think it hurts.

A big difference for add_license.sh is that the working directory is changed to the root ginkgo directory, and the temporary files will be created in dev_tools/scripts instead of inside the build directory. However, we should not have any name-clashes (and the temporary file names are now variables which can be easily altered).

Additionally, now all folders named build and third_party are ignored by add_license.sh, not just these folders inside the top level of the ginkgo folder (I changed that when I saw that it previously affected files in test_install/build on my system).

Performance wise, I ran time ./dev_tools/scripts/add_license.sh with the current status:

real	0m2,927s
user	0m2,957s
sys	0m1,046s

Before the changes (current develop status), it is:

real	0m3,007s
user	0m2,990s
sys	0m1,060s

So, we don't lose performance here.
Results for time ./dev_tools/scripts/update_ginkgo_header.sh with the current status:

real	0m0,055s
user	0m0,048s
sys	0m0,009s

Before:

real	0m0,055s
user	0m0,041s
sys	0m0,017s

So we don't lose performance here either.

TODO:

  • Confirm that it works on Mac

Closes #272.

@thoasm thoasm added is:bug Something looks wrong. reg:build This is related to the build system. labels Mar 24, 2019
@thoasm thoasm added this to the Ginkgo 1.0.0 release milestone Mar 24, 2019
@thoasm thoasm self-assigned this Mar 24, 2019
@thoasm thoasm added the 1:ST:ready-for-review This PR is ready for review label Mar 24, 2019
@hartwiganzt
Copy link
Collaborator

hartwiganzt commented Mar 25, 2019

Seems not to work for me:

Scanning dependencies of target generate_ginkgo_header
Scanning dependencies of target ginkgo_reference_device
Scanning dependencies of target ginkgo_cuda_device
Scanning dependencies of target ginkgo_omp_device
[  0%] Building CXX object core/devices/reference/CMakeFiles/ginkgo_reference_device.dir/dummy.cpp.o
[  1%] Building CXX object core/devices/cuda/CMakeFiles/ginkgo_cuda_device.dir/executor.cpp.o
[  1%] Building CXX object core/devices/omp/CMakeFiles/ginkgo_omp_device.dir/executor.cpp.o
find: -fprint: unknown primary or operator
Exiting due to an error being returned by `find`!
rm: global_includes.hpp.tmp: No such file or directory
make[2]: *** [CMakeFiles/generate_ginkgo_header] Error 1
make[1]: *** [CMakeFiles/generate_ginkgo_header.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....
[  1%] Built target ginkgo_reference_device
[  1%] Built target ginkgo_cuda_device
[  1%] Built target ginkgo_omp_device
make: *** [all] Error 2

@thoasm
Copy link
Member Author

thoasm commented Mar 25, 2019

@hartwiganzt Apparently, the -fprint option I was using is not supported on macs, which I did not know. I replaced this parameter with normal redirecting to a file, can you try it again?

@hartwiganzt
Copy link
Collaborator

Works now! Thanks!

@thoasm
Copy link
Member Author

thoasm commented Mar 25, 2019

Apparently, on Julius' Mac, the add_license.sh does not work (we manually changed the license in one of the files to check if it works) and get the following error multiple times:

grep: repetition-operator operand invalid

The update_ginkgo_header.sh seems to work fine on his system.

@thoasm
Copy link
Member Author

thoasm commented Mar 25, 2019

Now it works on Julius's Mac and on my system.
It would be nice if the reviewers could also test it on their systems to confirm that it works properly by just executing dev_tools/scripts/add_license.sh and dev_tools/scripts/update_ginkgo_header.sh. If there is no error message, it should work fine.

@hartwiganzt
Copy link
Collaborator

works.

Copy link
Member

@tcojean tcojean left a comment

Choose a reason for hiding this comment

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

LGTM

Thomas Grützmacher added 2 commits March 27, 2019 11:59
Removed the need for the following requested features in the scripts
`dev_tools/scripts/update_ginkgo_header.sh` and
`dev_tools/scripts/add_license.sh`:
- `extglob` (`**`) by using `find` instead
- `globstar` with appropriate arguments for `find`

Additionally, made sure that commands that are used (like `find`,
`sort`, ...) are present and do not return an error.
@thoasm thoasm force-pushed the bash_scripts_use_basic_functions branch from d4c025d to e7dca08 Compare March 27, 2019 11:05
`grep -F` prevents interpreting the given string as an expression
(treating `*`, `?` specially), which was necessary in the
`add_license.sh` because the beacon contains multiple `*`s and `<`.

Also added quotations for a `cd` in `update_ginkgo_header.sh`.
@thoasm thoasm force-pushed the bash_scripts_use_basic_functions branch from e7dca08 to 1a439b9 Compare March 27, 2019 11:11
@tcojean tcojean added 1:ST:ready-to-merge This PR is ready to merge. and removed 1:ST:ready-for-review This PR is ready for review labels Mar 27, 2019
@thoasm thoasm merged commit d05cd1a into develop Mar 27, 2019
@thoasm thoasm deleted the bash_scripts_use_basic_functions branch March 27, 2019 14:20
@thoasm thoasm mentioned this pull request Mar 27, 2019
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1:ST:ready-to-merge This PR is ready to merge. is:bug Something looks wrong. reg:build This is related to the build system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants