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.

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

Minor simplifications and clean-ups of build pipeline/workflows #2214

Merged
merged 1 commit into from
Apr 14, 2023

Conversation

HannesWell
Copy link
Member

This PR only contains a few minor simplifications of Xtext Github workflows and Jenkins pipeline that do not change the result.

@cdietrich cdietrich added this to the Release_2.31 milestone Apr 13, 2023
run: mvn clean verify -PuseJenkinsSnapshots -Dsurefire.rerunFailingTestsCount=3
if: matrix.os != 'ubuntu-latest'
working-directory: org.eclipse.xtext.full.releng
- name: Build and test
Copy link
Contributor

Choose a reason for hiding this comment

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

This actions seems to trigger the installation of additional packages in GitHub Actions:

Run coactions/setup-xvfb@v1
  with:
    run: mvn clean verify --show-version -PuseJenkinsSnapshots -Dsurefire.rerunFailingTestsCount=3
    working-directory: org.eclipse.xtext.full.releng
  env:
    JAVA_HOME: /opt/hostedtoolcache/Java_Temurin-Hotspot_jdk/17.0.6-10/x64
    JAVA_HOME_17_X64: /opt/hostedtoolcache/Java_Temurin-Hotspot_jdk/17.0.6-10/x64
/usr/bin/sudo apt-get install -y xvfb x11-xserver-utils
Reading package lists...
Building dependency tree...
Reading state information...
xvfb is already the newest version (2:21.1.3-2ubuntu2.9).
Suggested packages:
  nickle cairo-5c xorg-docs-core
The following NEW packages will be installed:
  x11-xserver-utils
0 upgraded, 1 newly installed, 0 to remove and 9 not upgraded.

while the "if" solution did not require any additional package installation.

Besides not having the "if" and 2 steps, does the use of this action provide additional benefits?
Have you used this action in other projects?

Copy link
Member

Choose a reason for hiding this comment

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

question would be: why is xvfb-run insufficient.
we used that cause the original GabrielBB/xvfb-action
was no longer maintained
coactions/setup-xvfb
seems to be the new fork.
but i am not sure if needed cause xvfb-run seem to work.
should this be reported to coactions/setup-xvfb

Copy link
Member Author

Choose a reason for hiding this comment

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

Besides not having the "if" and 2 steps, does the use of this action provide additional benefits?

No more benefits I'm aware of.

Have you used this action in other projects?

Yes, for example we used the predecessor (as Christian mentioned) in M2E for a while and I just changed to the new one recently: eclipse-m2e/m2e-core#1344
And SWT uses it as well (I should probably update it there too):
https://github.com/eclipse-platform/eclipse.platform.swt/blob/6912a16abe78c905180908ea4d59218f6519c272/.github/workflows/maven.yml#L60

question would be: why is xvfb-run insufficient.

Probably, yes. I have not digged into the details, but as far as I understand this is mainly for conveniences and avoids the need for the if.
From https://github.com/coactions/setup-xvfb

This action installs [XVFB](http://elementalselenium.com/tips/38-headless) and runs your headless tests with it. It cleans up the xvfb process after your tests are done. If it detects you're not using linux then your tests still run, but without xvfb, which is very practical for multi-platform workflows.

So that's probably it.

Copy link
Member

Choose a reason for hiding this comment

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

we were using the garielbb for rerference project integ tests nor about 5 years
but we stopped using once it was no longer maintained
and github was issuing warnings about the deprecation feature use of that
this is why we then switchted to the alternative approach
xtext/xtext-reference-projects@bc0256f

Copy link
Member Author

Choose a reason for hiding this comment

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

this is why we then switchted to the alternative approach

Understand, but since the action has been revived I think things changed a bit.
Any way, just let me know which way you want and I can act accordingly.

Copy link
Member

Choose a reason for hiding this comment

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

using the new action is ok. the question is the warning already reported upstream or do we do something wrong

Copy link
Member Author

Choose a reason for hiding this comment

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

OK. 👍🏽
Probably not reported, at least I cannot see any issue on that topic.
This extra package installation was just recently added in coactions/setup-xvfb#5. Unfortunately there is not much context.
Maybe this can be made smarter by just installing the recommended packages with it, but I'm not an apt expert?

Copy link
Member

Choose a reason for hiding this comment

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

hmmm ok

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the warning in GitHub Actions.
If the action has been revived, I'm OK with that.
The apt installation doesn't seem to take a lot of time.
If something goes wrong we can revert.
@cdietrich I'd approve the PR, what about you?

@HannesWell
Copy link
Member Author

Updated this PR to consider the changes made in #2211.

@LorenzoBettini LorenzoBettini merged commit 8591e76 into eclipse:main Apr 14, 2023
@HannesWell HannesWell deleted the minorBuildSimplifications branch April 14, 2023 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants