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

ci: murdock: Replace hardcoded path by bindir variable #8194

Merged
merged 1 commit into from
Dec 1, 2017

Conversation

bergzand
Copy link
Member

@bergzand bergzand commented Dec 1, 2017

Replaces the two rm -rf build to use the BINDIR variable.

@bergzand bergzand added Area: CI Area: Continuous Integration of RIOT components Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer labels Dec 1, 2017
@bergzand bergzand changed the title ci: murdock: Replace hardcoded path by bindir var ci: murdock: Replace hardcoded path by bindir variable Dec 1, 2017
@kaspar030 kaspar030 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 1, 2017
Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

ACK.

.murdock Outdated
@@ -99,7 +101,7 @@ compile() {
test -d ${BINDIR} && echo "-- build directory size: $(du -sh ${BINDIR} | cut -f1)"

# cleanup
rm -Rf build
rm -rf ${BINDIR}
Copy link
Member

@cgundogan cgundogan Dec 1, 2017

Choose a reason for hiding this comment

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

there's a test statement above. You should move the rm up there and concatenate with &&.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've made it a full if...then, otherwise that line would become a bit long.

@cgundogan
Copy link
Member

Much better now! Please squash.

@bergzand
Copy link
Member Author

bergzand commented Dec 1, 2017

@cgundogan after some tests I realized that rm -rf doesn't throw an error if the directory that is to be removed doesn't exist. Including it in the test statement makes sense, but is actually not necessary. The first rm -rf doesn't have a check whether the directory exists either. :)

@cgundogan
Copy link
Member

cgundogan commented Dec 1, 2017

after some tests I realized that rm -rf doesn't throw an error if the directory that is to be removed doesn't exist

yes I know. But why try to delete something that you are 100% sure of not being present? (:

@bergzand
Copy link
Member Author

bergzand commented Dec 1, 2017

Well, now it is checked twice, once by the bash script and once by the rm command :)

@cgundogan
Copy link
Member

And in the case the directory does not exist, it checks only once, not twice (;

@bergzand bergzand force-pushed the pr/murdock_cleanupvar branch from 8f3665d to 1a83c20 Compare December 1, 2017 14:54
@bergzand
Copy link
Member Author

bergzand commented Dec 1, 2017

I'm not going to make a huge point out of this, squashed

@cgundogan cgundogan merged commit 24476e0 into RIOT-OS:master Dec 1, 2017
@aabadie aabadie added this to the Release 2018.01 milestone Jan 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: CI Area: Continuous Integration of RIOT components CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants