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

Don't skip tests if there is an emulator #1808

Merged
merged 3 commits into from
Nov 28, 2023
Merged

Don't skip tests if there is an emulator #1808

merged 3 commits into from
Nov 28, 2023

Conversation

isuruf
Copy link
Member

@isuruf isuruf commented Nov 26, 2023 •
edited
Loading

Checklist

  • Added a news entry

@isuruf isuruf requested a review from a team as a code owner November 26, 2023 20:42
@isuruf isuruf mentioned this pull request Nov 26, 2023
Open
18 tasks
hmaarrfk
hmaarrfk reviewed Nov 26, 2023
View reviewed changes
conda_smithy/templates/run_win_build.bat.tmpl Outdated
if NOT [%HOST_PLATFORM%] == [%BUILD_PLATFORM%] (
set "EXTRA_CB_OPTIONS=%EXTRA_CB_OPTIONS% --no-test"
)
{%- endif %}
{%- if test == "native_and_emulated" %}
if NOT [%HOST_PLATFORM%] == [%BUILD_PLATFORM%] (
if NOT [%CROSSCOMPILING_EMULATOR%] == [1] (
Copy link
Contributor

Choose a reason for hiding this comment

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

I mentionned in conda-forge/conda-forge-ci-setup-feedstock#300 (comment) but I'm going to repeat the comment here:

On linux CROSSCOMPILING_EMULATOR is a path. On windows it is a boolean. I feel like it will be a problem later on.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we update the test to be:

if NOT [%CROSSCOMPILING_EMULATOR%] == []

in the case to give a prallel to the linux case, where non-emptyness means "valid"

@hmaarrfk hmaarrfk mentioned this pull request Nov 26, 2023
Merged
5 tasks
@hmaarrfk
Copy link
Contributor

i'm just making sure that you had found an error in the old logic? I hadn't caught it, but the new logic seems correct (if the syntax is valid)

@isuruf
Copy link
Member Author

isuruf commented Nov 26, 2023

Both are correct, but changed to checking emptyness.

hmaarrfk
hmaarrfk approved these changes Nov 26, 2023
View reviewed changes
Copy link
Contributor

@hmaarrfk hmaarrfk left a comment

Choose a reason for hiding this comment

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

This is good by me, but I haven't tested it with a real windows machine.

@isuruf
Copy link
Member Author

isuruf commented Nov 28, 2023

Thanks for the review

@isuruf isuruf merged commit d8ffc50 into conda-forge:main Nov 28, 2023
1 check passed
@isuruf
Copy link
Member Author

isuruf commented Nov 28, 2023

I tested this on CI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hmaarrfk hmaarrfk hmaarrfk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@isuruf @hmaarrfk