Skip to content

Allow integrations tests to run with selected backends - #1270

Merged
Smit-create merged 7 commits into
lcompilers:mainfrom
Smit-create:int_tests_back
Nov 10, 2022
Merged

Allow integrations tests to run with selected backends#1270
Smit-create merged 7 commits into
lcompilers:mainfrom
Smit-create:int_tests_back

Conversation

@Smit-create

@Smit-create Smit-create commented Nov 5, 2022

Copy link
Copy Markdown
Collaborator

After merging this we won't require integration_tests/run_tests.sh

@Smit-create Smit-create added the enhancement New feature or request label Nov 5, 2022
@ubaidsk

ubaidsk commented Nov 5, 2022

Copy link
Copy Markdown
Collaborator

Please also try to support this commit 0663126 in this new integration_tests/run_tests_new.py.

@ubaidsk

ubaidsk commented Nov 5, 2022

Copy link
Copy Markdown
Collaborator

Please also try to support this commit 0663126 in this new integration_tests/run_tests_new.py.

Or, maybe the CI itself can select which backends to run on which platforms with the new integration_tests/run_tests_new.py.

@Smit-create Smit-create mentioned this pull request Nov 5, 2022
Merged
@Smit-create
Smit-create marked this pull request as ready for review November 5, 2022 17:11
@Smit-create

Smit-create commented Nov 6, 2022

Copy link
Copy Markdown
Collaborator Author

The error is that the runtime_library path for lpython has some issues:

ltypes_path: /usr/share/miniconda/envs/test/bin/../share/lpython/lib/ltypes/ltypes.py
read_status: 0
ltypes_path: /home/runner/work/lpython/lpython/integration_tests/ltypes/ltypes.py
read_status: 0

@Thirumalai-Shaktivel

Thirumalai-Shaktivel commented Nov 6, 2022 •
edited
Loading

Copy link
Copy Markdown
Collaborator

I'm trying to debug the issue here. As you said, I think it's something related to the path!

@Thirumalai-Shaktivel

Thirumalai-Shaktivel commented Nov 6, 2022

Copy link
Copy Markdown
Collaborator

So, this works in your local machine, right?

@Smit-create

Smit-create commented Nov 6, 2022

Copy link
Copy Markdown
Collaborator Author

So, this works in your local machine, right?

Yes, it does!

@Smit-create

Smit-create commented Nov 6, 2022

Copy link
Copy Markdown
Collaborator Author

I'm trying to debug the issue here. As you said, I think it's something related to the path!

That has something to do with runtime_library_path in python_ast_to_asr.cpp

@Thirumalai-Shaktivel
Thirumalai-Shaktivel force-pushed the int_tests_back branch 3 times, most recently from 376d8f2 to 56918a0 Compare November 6, 2022 15:07
@Thirumalai-Shaktivel

Thirumalai-Shaktivel commented Nov 6, 2022

Copy link
Copy Markdown
Collaborator

Here is the issue:

Path: /usr/share/miniconda/envs/test/bin/lpython
DirName: /usr/share/miniconda/envs/test/bin

which is the lpython executable path.
Here:

lpython/src/lpython/utils.cpp

Lines 53 to 75 in d4a6d53

std::string get_runtime_library_dir()
{
char *env_p = std::getenv("LFORTRAN_RUNTIME_LIBRARY_DIR");
if (env_p) return env_p;
std::string path;
int dirname_length;
get_executable_path(path, dirname_length);
std::string dirname = path.substr(0,dirname_length);
if ( endswith(dirname, "src/bin")
|| endswith(dirname, "src\\bin")
|| endswith(dirname, "SRC\\BIN")) {
// Development version
return dirname + "/../runtime";
} else if (endswith(dirname, "src/lpython/tests") ||
endswith(to_lower(dirname), "src\\lpython\\tests")) {
// CTest Tests
return dirname + "/../../runtime";
} else {
// Installed version
return dirname + "/../share/lpython/lib";
}
}

@Thirumalai-Shaktivel
Thirumalai-Shaktivel force-pushed the int_tests_back branch 4 times, most recently from a4963b3 to 37ea579 Compare November 6, 2022 16:00
Thirumalai-Shaktivel
Thirumalai-Shaktivel reviewed Nov 6, 2022
View reviewed changes
Comment thread integration_tests/CMakeLists.txt Outdated
Comment on lines +62 to +63
COMMAND lpython -c ${CMAKE_CURRENT_SOURCE_DIR}/${name}.py -o ${name}.o
COMMAND "${CMAKE_SOURCE_DIR}/../src/bin/lpython" -c
${CMAKE_CURRENT_SOURCE_DIR}/${name}.py -o ${name}.o

@Thirumalai-Shaktivel Thirumalai-Shaktivel Nov 6, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Here is the bug! I hope this is the correct fix!

@Thirumalai-Shaktivel Thirumalai-Shaktivel Nov 6, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Perfect, All the tests passed!

@Smit-create Smit-create Nov 6, 2022

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, I just have a question that how does that work in current main then?

@Smit-create Smit-create Nov 6, 2022

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Ah, I get the exact bug now. It has to do with the PATH variable. In the current main branch we have the following:

% export PATH="$(pwd)/../src/bin:$PATH"

It adds the lpython executable path first and during the path search, we hit the expected path first.

@Smit-create

Smit-create commented Nov 6, 2022

Copy link
Copy Markdown
Collaborator Author

Thanks @Thirumalai-Shaktivel

@Smit-create Smit-create added the ready for review PRs that are ready for review label Nov 6, 2022
@Smit-create

Smit-create commented Nov 8, 2022

Copy link
Copy Markdown
Collaborator Author

@certik @czgdp1807 Please review this PR.

czgdp1807
czgdp1807 approved these changes Nov 8, 2022
View reviewed changes

@czgdp1807 czgdp1807 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is fine but I would suggest to wait until #1271 to be merged.

@Smit-create
Smit-create merged commit 0c003c9 into lcompilers:main Nov 10, 2022
@Smit-create
Smit-create deleted the int_tests_back branch November 10, 2022 05:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@czgdp1807 czgdp1807 czgdp1807 approved these changes
@ubaidsk ubaidsk ubaidsk approved these changes
@Thirumalai-Shaktivel Thirumalai-Shaktivel Thirumalai-Shaktivel approved these changes
@certik certik Awaiting requested review from certik

Assignees

No one assigned

Labels

enhancement New feature or request ready for review PRs that are ready for review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Smit-create @ubaidsk @Thirumalai-Shaktivel @czgdp1807