Skip to content

Fix remaining tests for Android #623

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

Merged
merged 1 commit into from
Jan 15, 2020
Merged

Conversation

finagolfin
Copy link
Member

Used to build and test llbuild and SPM natively on Android in the Termux app. I can modify lit.cfg to only check the PREFIX env var on Android if there's any worry that it might be spuriously set on other platforms.

makeTmpDir is only checked by the unit tests, which I noticed are not run by the CI anymore, maybe because those llbuild test binaries were moved around? I see the following warning on both the linux and mac CI for recent pulls:

lit.py: /home/buildnode/jenkins/workspace/swift-llbuild-PR-Linux-smoke-test/llvm-project/llvm/utils/lit/lit/discovery.py:210: warning: test suite 'llbuild-unit' contained no tests

Running them manually shows that no tests fail after this pull on Android.

char tmpDirPathBuf[] = "/tmp/fileXXXXXX";
char *tmpDirPathBuf = nullptr;
if (const char *tmpDir = std::getenv("TMPDIR"))
asprintf(&tmpDirPathBuf, "%s/fileXXXXXX", tmpDir);
Copy link
Contributor

Choose a reason for hiding this comment

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

The buffer allocated here needs to be free()d before we return.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

else
tmpDirPath = "/tmp/fileXXXXXX";
auto tmpDirString = std::string(mkdtemp(tmpDirPath));
free(tmpDirPath);
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 This may now attempt to free the inline "/tmp/fileXXXXXX".

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 think this last iteration should finally work fine.

@dmbryson
Copy link
Contributor

@swift-ci please smoke test

@finagolfin
Copy link
Member Author

Hmm, I didn't know if the PackageDescription version would make a difference like it did natively on Android, swiftlang/swift-package-manager#2466, guess it does so removed that Android addition for ncurses till the manifest version supports it.

@dmbryson
Copy link
Contributor

@swift-ci please smoke test

1 similar comment
@dmbryson
Copy link
Contributor

@swift-ci please smoke test

@finagolfin finagolfin changed the title Fix SPM manifest and remaining tests for Android Fix remaining tests for Android Jan 14, 2020
@dmbryson dmbryson merged commit 36c0d5d into swiftlang:master Jan 15, 2020
@finagolfin finagolfin deleted the droid branch January 16, 2020 09:11
@finagolfin
Copy link
Member Author

Thanks, about the unit tests, I notice that warning: test suite 'llbuild-unit' contained no tests in both the linux and mac CI logs for this pull, but I now see that the mac CI circles back around at the end and runs the unit tests some other way, though they still don't appear to be run on the linux CI. Oversight or conscious choice?

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.

2 participants