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.

Sign up for GitHub

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

Jump to bottom

[utils] Add getRealpath function #2050

Merged
merged 1 commit into from
Dec 1, 2022
Merged

[utils] Add getRealpath function #2050

merged 1 commit into from
Dec 1, 2022

Conversation

baek2sm
Copy link
Contributor

@baek2sm baek2sm commented Nov 23, 2022 •
edited
Loading

Since the "realpath" function is not available in the Windows operating system, the "getRealpath" function has been added and replaced so that it can be used in the Windows operating system.

  • Add "getRealpath" function to utils.

Self evaluation:

  1. Build test: [x]Passed []Failed []Skipped
  2. Run test: [x]Passed []Failed []Skipped

Signed-off-by: Seungbaek Hong sb92.hong@samsung.net

@taos-ci
Copy link

taos-ci commented Nov 23, 2022

📝 TAOS-CI Version: 1.5.20200925. Thank you for submitting PR #2050. Please a submit 1commit/1PR (one commit per one PR) policy to get comments quickly from reviewers. Your PR must pass all verificiation processes of cibot before starting a review process from reviewers. If you are new member to join this project, please read manuals in documentation folder and wiki page. In order to monitor a progress status of your PR in more detail, visit http://ci.nnstreamer.ai/.

@baek2sm
Copy link
Contributor Author

baek2sm commented Nov 23, 2022

I will check the CI errors tomorrow and reflect the corrections.

lhs8928
lhs8928 approved these changes Nov 23, 2022
View reviewed changes
Copy link
Contributor

@lhs8928 lhs8928 left a comment

Choose a reason for hiding this comment

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

LGTM

nntrainer/utils/util_func.cpp Outdated Show resolved Hide resolved
taos-ci
taos-ci approved these changes Nov 23, 2022
View reviewed changes
Copy link

@taos-ci taos-ci left a comment

Choose a reason for hiding this comment

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

@baek2sm, 💯 All CI checkers are successfully verified. Thanks.

jijoongmoon
jijoongmoon reviewed Nov 24, 2022
View reviewed changes
nntrainer/utils/util_func.cpp
@@ -197,4 +197,12 @@ bool istrequal(const std::string &a, const std::string &b) {
});
}

char *get_realpath (const char *__restrict __name, char *__restrict __resolved) {
#ifdef _WIN32
Copy link
Collaborator

@jijoongmoon jijoongmoon Nov 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

How about change the def for the consistency.
for Android, nntrainer uses __ANDROID___, Tizen, ___TIZEN__.
So.. for Windows, how about ___WIN32___ ?

Copy link
Contributor Author

@baek2sm baek2sm Nov 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you, I didn't know that there was a predefined keyword called __WIN32__. I will fix it!

Copy link
Member

@myungjoo myungjoo Nov 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

_WIN32 is predefined macro defined by toolchain. __ANDROID__ and __TIZEN__ are macro defined by nntrainer. If you want __WIN32__, you will need to do it in a common header: #define __WIN32__ (_WIN32) (do we really need this?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This modification has not been reflected yet. If it is necessary to modify the keyword _WIN32 into __WIN32__, then I will modify and reflect it again.

Copy link
Member

Choose a reason for hiding this comment

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

OR... maybe https://en.cppreference.com/w/cpp/filesystem/absolute is better?

Copy link
Collaborator

@jijoongmoon jijoongmoon Nov 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

_WIN32 is predefined macro defined by toolchain

if it is defined by Visual Studio, then it is ok to go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OR... maybe https://en.cppreference.com/w/cpp/filesystem/absolute is better?

The "absolute" function seems to work well in windows. It seems to be best way to use the absolute function. I will reflect this modification.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modifications were reflected to use the "absolute" function. At this time, the existing PR has been modified as it is, but I wonder if it would be better to upload new PR in the future if it is completely different from the existing PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filesystem including absolute function does not seem to be fully supported in android r21 (supported from r22) : android/ndk#609 (comment). Therefore, I would like to modify it again by defining the "getRealpath" function as before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I uploaded it reflecting the work situation so far.

@jijoongmoon
Copy link
Collaborator

Plz, Check the CI.

DonghakPark
DonghakPark approved these changes Nov 24, 2022
View reviewed changes
Copy link
Member

@DonghakPark DonghakPark left a comment

Choose a reason for hiding this comment

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

LGTM!

taos-ci
taos-ci approved these changes Nov 24, 2022
View reviewed changes
Copy link

@taos-ci taos-ci left a comment

Choose a reason for hiding this comment

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

@baek2sm, 💯 All CI checkers are successfully verified. Thanks.

taos-ci
taos-ci approved these changes Nov 24, 2022
View reviewed changes
Copy link

@taos-ci taos-ci left a comment

Choose a reason for hiding this comment

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

@baek2sm, 💯 All CI checkers are successfully verified. Thanks.

taos-ci
taos-ci approved these changes Nov 25, 2022
View reviewed changes
Copy link

@taos-ci taos-ci left a comment

Choose a reason for hiding this comment

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

@baek2sm, 💯 All CI checkers are successfully verified. Thanks.

@SeoHyungjun
Copy link
Member

LGTM

@baek2sm baek2sm changed the title [utils] Add get_realpath function [WIN32] Replace realpath function with absolute function Nov 30, 2022
@baek2sm baek2sm force-pushed the windows_build branch 3 times, most recently from cd1c678 to 49d4ddd Compare November 30, 2022 06:09
@taos-ci
Copy link

taos-ci commented Nov 30, 2022

:octocat: cibot: @baek2sm, A builder checker could not be completed because one of the checkers is not completed. In order to find out a reason, please go to http://ci.nnstreamer.ai/nntrainer/ci/repo-workers/pr-checker/2050-202211301509200.16678190231323-49d4ddd53fe185783fbff0263d95683654db6764/.

[utils] Add getRealpath function
30fae59 
Since the "realpath" function is not available in the Windows operating system, the "getRealpath" function has been added and replaced so that it can be used in the Windows operating system.

- Add "getRealpath" function to utils.

**Self evaluation:**
1. Build test: [x]Passed []Failed []Skipped
2. Run test: [x]Passed []Failed []Skipped

Signed-off-by: Seungbaek Hong <sb92.hong@samsung.net>
@baek2sm baek2sm changed the title [WIN32] Replace realpath function with absolute function [utils] Add getRealpath function Nov 30, 2022
taos-ci
taos-ci approved these changes Nov 30, 2022
View reviewed changes
Copy link

@taos-ci taos-ci left a comment

Choose a reason for hiding this comment

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

@baek2sm, 💯 All CI checkers are successfully verified. Thanks.

@jijoongmoon jijoongmoon merged commit 6f82ee6 into nnstreamer:main Dec 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jijoongmoon jijoongmoon jijoongmoon left review comments

@jihochu jihochu jihochu left review comments

@DonghakPark DonghakPark DonghakPark approved these changes

@taos-ci taos-ci taos-ci approved these changes

@lhs8928 lhs8928 lhs8928 approved these changes

@myungjoo myungjoo Awaiting requested review from myungjoo myungjoo is a code owner

@again4you again4you Awaiting requested review from again4you again4you is a code owner

@jaeyun-jung jaeyun-jung Awaiting requested review from jaeyun-jung jaeyun-jung is a code owner

@leemgs leemgs Awaiting requested review from leemgs leemgs is a code owner

@wooksong wooksong Awaiting requested review from wooksong wooksong is a code owner

@helloahn helloahn Awaiting requested review from helloahn

@kparichay kparichay Awaiting requested review from kparichay

@gichan-jang gichan-jang Awaiting requested review from gichan-jang gichan-jang is a code owner

@anyj0527 anyj0527 Awaiting requested review from anyj0527 anyj0527 is a code owner

@zhoonit zhoonit Awaiting requested review from zhoonit

Assignees
No one assigned
Labels
BUILD/CI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@baek2sm @taos-ci @jijoongmoon @SeoHyungjun @myungjoo @jihochu @lhs8928 @DonghakPark