Skip to content

Conversation

@rschatz
Copy link
Member

@rschatz rschatz commented Jun 15, 2021

Trying to fix #3423

@rschatz rschatz force-pushed the rs/fix-github-actions-pylint branch from e527df2 to a66ac44 Compare June 15, 2021 15:53
@rschatz rschatz changed the title Fix pylint check in github actions workflow. Fix github actions failures. Jun 15, 2021
@rschatz rschatz marked this pull request as ready for review June 15, 2021 16:28
@rschatz
Copy link
Member Author

rschatz commented Jun 15, 2021

Also added an attempted fix for #3433

Basically, we're just doing rm -rf .git and then run mx gate --tags build (which is roughly equivalent to mx clean && mx build) in the substratevm directory.

@zakkak can you verify this is what the WITHOUT_VCS=true gate is supposed to test? If that's ok, I'll also add a test to our internal gate that does the same thing, to prevent future breakage (or do we have that already? cc @olpaw )

${MX_PATH}/mx --primary-suite-path ${PRIMARY} --J @"-Xmx2g" --java-home=${JAVA_HOME} gate --strict-mode --tags ${GATE}
fi
${MX_PATH}/mx --primary-suite-path ${PRIMARY} --J @"-Xmx2g" --java-home=${JAVA_HOME} gate --strict-mode --tags ${GATE}
Copy link
Member

@olpaw olpaw Jun 15, 2021

Choose a reason for hiding this comment

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

This will build a GraalVM release that contains the /substratevm suite.
After running this,
$(mx --primary-suite-path ${PRIMARY} graalvm-home) contains a working bin/native-image bash-launcher.
i.e. this looks good.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately this is not enough to test if things work outside a VCS repo, please see #3478 (comment) for more details.

Copy link
Member

@olpaw olpaw Jun 16, 2021

Choose a reason for hiding this comment

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

Unfortunately this is not enough to test if things work outside a VCS repo

Since 6eee91e got merged almost all substrate-related mx commands (like e.g. mx native-image or mx native-image-configure) are just calling artifacts that got built as part of running mx build in the substratevm directory. In other words if mx build (or mx gate --tags build) succeeded you can safely assume that mx native-image --help also works since its just calling the native-image bash launcher that is located at $(mx --primary-suite-path ${PRIMARY} graalvm-home)/bin/native-image after mx build made sure it got created there.

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 guess we could also do this:

PRIMARY=vm
DYNAMIC_IMPORTS=/substratevm,/sulong,/tools
NATIVE_IMAGES=polyglot # to save some time
GATE=build

It's not literally building everything, but it would exercise almost every different category of stuff there is to build (e.g. Java projects, native projects, a native-image launcher, ...).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, I admit I didn't check the latest state of mx_substratevm.py, my mistake. Thanks for making this clear Paul.

Copy link
Member

Choose a reason for hiding this comment

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

It's not literally building everything, but it would exercise almost every different category of stuff there is to build (e.g. Java projects, native projects, a native-image launcher, ...).

Yup that would give pretty good coverage.

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, and seems to be working (at least on github, still waiting for the internal gate to finish).

@zakkak
Copy link
Collaborator

zakkak commented Jun 15, 2021

Also added an attempted fix for #3433

Basically, we're just doing rm -rf .git and then run mx gate --tags build (which is roughly equivalent to mx clean && mx build) in the substratevm directory.

@zakkak can you verify this is what the WITHOUT_VCS=true gate is supposed to test? If that's ok, I'll also add a test to our internal gate that does the same thing, to prevent future breakage (or do we have that already? cc @olpaw )

Unfortunately, as stated in #2738 (comment) mx gate --tags build does not suffice.

Looking at the changes in 05d53b0 one sees that only 3 suites are affected (sdk, compiler, substratevm).
Now in these suites only a subset of the commands are affected. For instance in substratevm the only affected commmands are native-image and native-image-configure (the ones invoking _ensure_vm_built. As a result,
mx --primary-suite-path substratevm gate --tags build works fine even without this PR.

In order to test all the changes of this PR one needs to run:

mx --primary-suite-path substratevm native-image -version
mx --primary-suite-path compiler makegraaljdk /tmp/graaljdk
mx --primary-suite-path sdk javadoc

but this still allows people to perform changes that could break mx commands performed outside a vcs repository.
E.g. mx --primary-suite-path substratevm build

@olpaw
Copy link
Member

olpaw commented Jun 16, 2021

Unfortunately, as stated in #2738 (comment) mx gate --tags build does not suffice.

This is not true anymore. _ensure_vm_built got removed as part committing 6eee91e

So with my changes on master it is now indeed sufficient for testing if building without VCS works to run mx gate --tags build

@zakkak
Copy link
Collaborator

zakkak commented Jun 16, 2021

I see, I admit I didn't check the latest state of mx_substratevm.py, my mistake. Thanks for making this clear Paul.

Copy link
Collaborator

@zakkak zakkak left a comment

Choose a reason for hiding this comment

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

Thanks for the fix @rschatz

@rschatz
Copy link
Member Author

rschatz commented Jun 16, 2021

Unfortunately, as stated in #2738 (comment) mx gate --tags build does not suffice.

Ah, I see, that explains my confusion. Also good to hear that it's not necessary anymore ;)

@rschatz rschatz force-pushed the rs/fix-github-actions-pylint branch from a0adce7 to fc459f5 Compare June 16, 2021 22:56
@graalvmbot graalvmbot merged commit 6e8d86b into oracle:master Jun 22, 2021
@zakkak
Copy link
Collaborator

zakkak commented Jun 22, 2021

Thanks @rschatz

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GraalVM gate CI workflow failing due to a pylint check in mx_sulong_suite_constituents.py

4 participants