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

Run GH verification build on Windows too #1163

Merged
merged 1 commit into from
Aug 27, 2024

Conversation

HannesWell
Copy link
Contributor

Follow up of #1081 to enable verification builds for Windows once actions/upload-artifact#240 is resolved.

Once eclipse-tycho/tycho#1891 is resolved disabling the tycho-baseline for Windows should also not be necessary anymore.

@laeubi
Copy link
Member

laeubi commented Dec 31, 2022

Can't we just disable automatic conversion of line-endings anyways? I'm not aware of any file where it would be relevant?

@HannesWell
Copy link
Contributor Author

Can't we just disable automatic conversion of line-endings anyways? I'm not aware of any file where it would be relevant?

For the CI we could do what is suggested int actions/checkout#135.

But we should not do that for the repo in general, because if you only checkout linux-style line-endings on windows over the time most editors will mess that up. Therefore for local builds Tycho should be made smarter in that regard.

@laeubi
Copy link
Member

laeubi commented Dec 31, 2022

if you only checkout linux-style line-endings on windows over the time most editors will mess that up

Maybe we should the fix this "editors" (aka Eclipse) :-)

@github-actions
Copy link

github-actions bot commented Dec 31, 2022

Test Results

  321 files  +107    321 suites  +107   28m 46s ⏱️ + 11m 45s
  665 tests ±  0    652 ✅  -   2  10 💤 ± 0  2 ❌ +1  1 🔥 +1 
1 995 runs  +665  1 962 ✅ +653  30 💤 +10  2 ❌ +1  1 🔥 +1 

For more details on these failures and errors, see this check.

Results for commit d4d1e47. ± Comparison against base commit a4387fc.

♻️ This comment has been updated with latest results.

@HannesWell
Copy link
Contributor Author

if you only checkout linux-style line-endings on windows over the time most editors will mess that up

Maybe we should the fix this "editors" (aka Eclipse) :-)

That will probably be a big task. But it is not only Eclipse. It is just every other 'text'-editor you can use. Windows notebap, Notepad++, etc. We can't 'fix' them all. And since CRLF simply is the default line-ending on Windows I'm not even sure it would be a real fix or would just cause more confusion. The root problem is the discrepancy in line-ending between Windows and POSIX OS. But I don't think we have the power to convince the Windows devs to change that (although it would solve many problems). :/

@laeubi
Copy link
Member

laeubi commented Mar 6, 2023

@HannesWell can you give a status here? If there are only baseline issues, I think we should probabbly just merge this and then open a ticket at Tycho for every remaining issue to track progress.

@HannesWell
Copy link
Contributor Author

Follow up of #1081 to enable verification builds for Windows once actions/upload-artifact#240 is resolved.

As far as I can tell, the main issue actions/upload-artifact#240 is still not resolved, but lets see what the build says.

@HannesWell
Copy link
Contributor Author

Current failure on Windows is

Error:  Failed to execute goal org.eclipse.tycho:tycho-p2-plugin:4.0.0-SNAPSHOT:p2-metadata (p2-metadata) on project org.eclipse.m2e.maven.runtime: baseline and build artifacts have same version but different contents
Error:     no-classifier: different
Error:        jars/jansi-2.4.0.jar: different
Error:           org/fusesource/jansi/internal/native/Mac/arm64/libjansi.jnilib: different
Error:           org/fusesource/jansi/internal/native/Mac/x86/libjansi.jnilib: different
Error:           org/fusesource/jansi/internal/native/Mac/x86_64/libjansi.jnilib: different

Have not yet checked it, but if I'd have guess, I would say that the windows file sparators are not taken into account in the ignoredPatterns configuration of the tycho-p2-plugin:p2-metadata goal.

@laeubi
Copy link
Member

laeubi commented Apr 19, 2023

Have not yet checked it, but if I'd have guess, I would say that the windows file sparators are not taken into account in the ignoredPatterns configuration of the tycho-p2-plugin:p2-metadata goal.

It does not seem to use native seperators, and jar files should always use / so are these really ignored? maybe it is more because of case invariant and we probabbly should just use:

**/libjansi.jnilib

as a pattern (would then be required in the tycho-baseline plugin as well)?!?

@laeubi
Copy link
Member

laeubi commented Jun 4, 2023

@HannesWell do you think we should finish this one? I have adapted Tycho now since a while that it should handle line endings more graceful

@HannesWell
Copy link
Contributor Author

@HannesWell do you think we should finish this one? I have adapted Tycho now since a while that it should handle line endings more graceful

Looks like the ignored path is still not working. I remember to have looked into that at Tycho while ago but then I somehow was not able to reproduce that (although I'm on Windows). I'll look at that again.

Nevertheless I think we should also apply your fix from P2's eclipse-equinox/p2#288 here, since I remember that we faced a similar issue.

Comment on lines 24 to 39
<properties>
<product-extension>tar.gz</product-extension>
</properties>
<profiles>
<profile>
<id>windows</id>
<activation>
<os>
<family>windows</family>
</os>
</activation>
<properties>
<product-extension>zip</product-extension>
</properties>
</profile>
</profiles>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ptziegler do you know a more elegant solution than this one to adapt the AUT to the different file-extensions used for the archives?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not aware of any way RCPTT can automatically detect the type of archive. Though I wonder if it would be less of a hassle to simply rewrite the test to use e.g. SWTBot...

Copy link
Member

Choose a reason for hiding this comment

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

I think for our purpose it would be okay to always use tar.gz in all cases if it helps (we don't offer the products a user downloads where it would be a bit inconvenient).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not aware of any way RCPTT can automatically detect the type of archive. Though I wonder if it would be less of a hassle to simply rewrite the test to use e.g. SWTBot...

I have no clue about this. I have neither worked with RCPTT nor with SWTBot yet, so I fully trust your assessment on this topic.

I think for our purpose it would be okay to always use tar.gz in all cases if it helps (we don't offer the products a user downloads where it would be a bit inconvenient).

Yes I thought about this too, but was hoping there might be a more elegant solution.
But lets to it this way for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have no clue about this. I have neither worked with RCPTT nor with SWTBot yet, so I fully trust your assessment on this topic.

The advantage of SWTBot is that it's executed via JUnit, so you don't really have to deal with file extensions, let alone the assembled product.

@HannesWell
Copy link
Contributor Author

Finally.
With all baseline checks disabled for Windows the workflow succeeds now (I hope #1733 allow to reduce the number of workarounds), besides two test failures. I see if I can fix them in the next days, but otherwise this seems ready for submission.

@HannesWell HannesWell marked this pull request as ready for review August 26, 2024 22:21
@HannesWell
Copy link
Contributor Author

I have checked the tests failing on Windows and they are either not reproducible or not easy to fix.
But since I think it is more valuable to have an important platform tested with two failing tests than not tested at all, I'll submit this now and fix the failing tests in a follow-up.
We can then also step by step remove the workarounds when possible.

@HannesWell HannesWell merged commit 64e54d0 into eclipse-m2e:master Aug 27, 2024
6 of 8 checks passed
@HannesWell HannesWell deleted the matrixBuildWindows branch August 27, 2024 20:39
@HannesWell HannesWell added this to the 2.6.2 milestone Sep 3, 2024
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.

3 participants