Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[ios][tests] Run functional tests with Mono and Native AOT on Helix #87773
[ios][tests] Run functional tests with Mono and Native AOT on Helix #87773
Changes from 82 commits
74481af
ca86967
98522fd
6ade906
0e7a920
1aca721
774ed6b
034fff1
a12779b
abfe7c1
09c6097
063995d
3693155
5344255
7c426a5
30ae9eb
fff4849
df51223
0e7325e
ce43877
6304c83
6eb91b7
228ba32
f42f06a
a667de7
e0ec3aa
a681087
eb23036
a811966
9374010
44f40ca
8d47870
2eecc7e
ae8ea04
f168689
72053e0
a5c8f8b
5ac774f
9b5641c
048a5b9
13fa800
8a46827
c8515cd
d715082
99cc75b
d0911d3
7a9cace
fd39b23
a826dbe
4438806
9e06c1f
29911b9
6a420d3
d2d83b0
917ce00
dc52f2f
9bb44e6
cd8a7f1
e7e4d1a
77f7cf4
b7b285e
486c1a3
0d7d88b
8470e8f
454f40f
e260892
e50a647
f3d9b25
f14e668
297c69f
751a911
3f28fd1
1ff7d9d
bbcc827
d8e7ebd
25a057c
cedc3f0
a2d57da
c06f211
161e45f
d992084
6d13f76
3517646
a35bbb7
05170f9
295d640
5d7af31
989cbca
3b20520
d092326
d4a3a56
3b62fc8
36368e2
496acec
aa66bb1
e47d986
cf56983
26955e0
7637762
e152e73
4af46f8
8a59bca
7dd5b27
8663185
2238ba2
cc5313a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have the capacity to run these on every PR. Even when we kick off runtime-ioslike manually, there may be a capacity issue running both mono and nativeaot together.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, my concern was all libraries tests (when we get to it) and I see it's running smoke tests only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clarify: You removed
/p:MonoEnableLLVM=true
because it is supposed to be set in the project file of the functional test?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, it should be set in the project file.Correct, it should be set in the project file in the Helix payload.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, I assume
/p:MonoForceInterpreter=$(MonoForceInterpreter)
could be also removed?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When AOT compiling on Helix, there is a set of parameters passed when the script is invoked, such as RunAOTCompilation, UseNativeAOTRuntime, TargetOS, TargetArchitecture. These parameters are usually set through the build command. Additionally, there are parameters that configure AOT compilation like EnableLLVM, IncludesTestRunner, UseRuntimeComponents, etc.
The MonoForceInterpreter parameter is passed through the build command, so it may be reasonable to keep it within the build command on the Helix as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. We should verify what is happening during the build of functional tests, as in functional tests project files the property is treated with:
TreatAsLocalProperty="MonoForceInterpreter"
which afaik, means the build system will not allow overwritingMonoForceInterpreter
value as long as it is defined in the project file - locally.So since
MonoForceInterpreter=false
got removed from the project file: https://github.com/dotnet/runtime/pull/87773/files#diff-67da87e0a0169c2fa25ef2d8106257412638ac6a05f4534bab3a62469fdf73aeL5doing
build -p:MonoForceInterpreter=true
on Helix - could build the test project differently then before.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the other functional tests project files, most of them have
MonoForceInterpreter
set, but some haveTreatAsLocalProperty="MonoForceInterpreter"
some don't, so I believe theTreatAsLocalProperty
was added as a workaround to prevent overwriting what a project specifies through the command line - like building on Helix.To conclude, I feel like this needs to be refactored (maybe not in this PR), with either removing any notion of
MonoForceInterpreter
from the functional test's project file (and test the change), or leave it how it was to prevent any unwanted regression.@steveisok what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a related change #52606. My understanding is that it was added for local development and testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the relevant tests have passed. @steveisok What do you suggest doing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ivanpovazan Reverted the changes to be on the safe side. Please take a look again at this PR and let's try to resolve comments before the RC1 snap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest keeping it and slowly unwinding in a follow up.