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

Add override flag for setting WORKING_DIRECTORY #508

Merged
merged 1 commit into from
May 22, 2020

Conversation

HamedSabri-adsk
Copy link
Contributor

Added a new flag to allow overriding WORKING_DIRECTORY. If this flag is not set, ${CMAKE_CURRENT_SOURCE_DIR} is used hence the default behavior.

UFE tests are supposed to run from build directory. We already copy all the python tests and resource to build directory and therefore WORKING_DIRECTORY needs to be set to ${CMAKE_CURRENT_BINARY_DIR}

@HamedSabri-adsk HamedSabri-adsk added the unit test Related to unit tests (both python or c++) label May 20, 2020
@HamedSabri-adsk HamedSabri-adsk requested a review from pmolodo May 20, 2020 20:54
Comment on lines +78 to +84
# set the working_dir
if(PREFIX_WORKING_DIRECTORY)
set(working_dir ${PREFIX_WORKING_DIRECTORY})
else()
set(working_dir ${CMAKE_CURRENT_SOURCE_DIR})
endif()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that I look at this again, I realize this can simplified. I will make a commit to address this.
if(NOT PREFIX_WORKING_DIRECTORY)
set(working_dir ${CMAKE_CURRENT_SOURCE_DIR})
endif()

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... I don't quite understand your fix - I think the set(working_dir ${PREFIX_WORKING_DIRECTORY}) is needed, isn't it? Or are there other changes, too? I guess I'll wait for the actual commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@elrond79 Sorry, Yes ignore my comment here. I am not sure what I was thinking when I wrote this comment. It has been a long day......

Everything as is should be fine. Thanks.

Copy link
Contributor

@pmolodo pmolodo left a comment

Choose a reason for hiding this comment

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

Hey, so as is, this looks good to me. Only note was where Hamed said he was going to tighten something up, and I didn't quite follow his comment - so I'll just wait on the actual fix. But I'd be fine with merging as-is.

Comment on lines +78 to +84
# set the working_dir
if(PREFIX_WORKING_DIRECTORY)
set(working_dir ${PREFIX_WORKING_DIRECTORY})
else()
set(working_dir ${CMAKE_CURRENT_SOURCE_DIR})
endif()

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... I don't quite understand your fix - I think the set(working_dir ${PREFIX_WORKING_DIRECTORY}) is needed, isn't it? Or are there other changes, too? I guess I'll wait for the actual commit.

@kxl-adsk kxl-adsk merged commit ffd3e13 into dev May 22, 2020
@kxl-adsk kxl-adsk deleted the sabrih/add_ovveride_working_directory branch May 22, 2020 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
unit test Related to unit tests (both python or c++)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants