Skip to content

fix guess_start_dir in case we're already in start_dir#1415

Merged
boegel merged 12 commits intoeasybuilders:developfrom
boegel:fix_guess_start_dir
Oct 12, 2015
Merged

fix guess_start_dir in case we're already in start_dir#1415
boegel merged 12 commits intoeasybuilders:developfrom
boegel:fix_guess_start_dir

Conversation

@boegel
Copy link
Copy Markdown
Member

@boegel boegel commented Oct 7, 2015

follow-up for #1413, since that was a non-backwards compatible change (I realised that the second after hitting the shiny green Merge button)

@wpoely86: can you review and maybe look into an enhanced unit test for this?

@hpcugentbot
Copy link
Copy Markdown

EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2127/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

print self.cfg['start_dir'] here as that is the dir that is actually specified?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

no, this error applies to the failing if os.path.exists(abs_start_dir), but I'll include an extra debug log message printing the start_dir we end up with

@wpoely86
Copy link
Copy Markdown
Member

wpoely86 commented Oct 7, 2015

Can this not be done simpler by letting the extract_step not change the directory but simple setting the finaldir?

@boegel
Copy link
Copy Markdown
Member Author

boegel commented Oct 7, 2015

@wpoely86: you're working on a test for this? PR pending?

@wpoely86
Copy link
Copy Markdown
Member

wpoely86 commented Oct 7, 2015

@boegel I've sent you a PR with it: boegel#10

@wpoely86
Copy link
Copy Markdown
Member

wpoely86 commented Oct 7, 2015

Btw, it can not be done simpler. This is probably the best solution.

@boegel
Copy link
Copy Markdown
Member Author

boegel commented Oct 7, 2015

@wpoely86: I fleshed out the test you wrote a bit more, please rererereview?

Should be good to go in otherwise, imho.

@wpoely86
Copy link
Copy Markdown
Member

wpoely86 commented Oct 7, 2015

looks good

@hpcugentbot
Copy link
Copy Markdown

EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2135/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

@hpcugentbot
Copy link
Copy Markdown

EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2136/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

@hpcugentbot
Copy link
Copy Markdown

EasyBuild framework unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2173/console for more details.

Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do.

@hpcugentbot
Copy link
Copy Markdown

EasyBuild framework unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2174/console for more details.

Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do.

@hpcugentbot
Copy link
Copy Markdown

EasyBuild framework unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2176/console for more details.

Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do.

Comment thread easybuild/tools/filetools.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the -1 is not needed?

@hpcugentbot
Copy link
Copy Markdown

EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2177/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

add two absolute paths with and without overlap?

@wpoely86
Copy link
Copy Markdown
Member

lgtm

@hpcugentbot
Copy link
Copy Markdown

EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2178/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

@boegel
Copy link
Copy Markdown
Member Author

boegel commented Oct 12, 2015

Going in, thanks for the review @wpoely86!

boegel added a commit that referenced this pull request Oct 12, 2015
fix guess_start_dir in case we're already in start_dir
@boegel boegel merged commit fc92aa8 into easybuilders:develop Oct 12, 2015
@boegel boegel deleted the fix_guess_start_dir branch October 12, 2015 21:11
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.

3 participants