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

Actions build #3635

Merged
merged 13 commits into from
Dec 9, 2020
Merged

Actions build #3635

merged 13 commits into from
Dec 9, 2020

Conversation

jburel
Copy link
Member

@jburel jburel commented Nov 16, 2020

This PR introduces support for GH actions, upgrade to Python 3 and deprecate variable in UpgradeChecker.
GH actions only run the build with ant and maven in two separate files.
I had to fix code/test to pass on Windows.

cc @sbesson

see https://github.com/jburel/bioformats/actions/runs/366417979 and https://github.com/jburel/bioformats/actions/runs/366417976

@jburel jburel mentioned this pull request Nov 16, 2020
String p = f.getParent();
String n = f.getName();
return new Location(p, n + ".bfoptions").getAbsolutePath();
return new Location(id + ".bfoptions").getAbsolutePath();
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain what behavior you are trying to fix on Windows here?

Copy link
Member Author

@jburel jburel Nov 17, 2020

Choose a reason for hiding this comment

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

\t1.tiff on windows will be turned into \\t1.tiff.bfoptions instead of D:\t1.tiff.bfoptions.
The case \foo\t1.tiff works as expected without the change

Copy link
Member

@sbesson sbesson Nov 17, 2020

Choose a reason for hiding this comment

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

Immediate thought is that \foo\t1.tiff is not a valid Windows path anyways no? The alternate approach should be to update the unit test data provider to work on both platforms.

Copy link
Member Author

@jburel jburel Nov 17, 2020

Choose a reason for hiding this comment

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

The tests are only for linux so I went for the approach of replacing / by \ as I had to do for the other sets of tests and use Location to be "correct" since it is not the current drive that is returned i.e. D: is returned when using GH actions
What was the rational behind the split in the first place?

Copy link
Member Author

Choose a reason for hiding this comment

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

\foo\t1.tiff will be valid from the current drive

Copy link
Member

Choose a reason for hiding this comment

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

@dgault: can you comment on the use of new Location(String, String) versus new Location(String). I seem to remember a few discussions around this but only found #2914

@sbesson
Copy link
Member

sbesson commented Nov 18, 2020

Supplementing the discussion on the API in #3635 (comment), a few comments on the migration:

  • overall 👍 for starting the migration. Builds seem to indicate we can have some good coverage of platforms/Java versions
  • the Ant workflow makes sense to keep as a standalone workflow as it is specific to this repository
  • re the Maven workflow, the parallel review Gradle build .github#12 made me wonder whether we would like to turn it into a generic workflow that we could use across all Maven-based repos, i.e. checkout, setup java, cache, run maven...
  • the only remaining element in .travis.yml not covered here is the deployment of the artifacts to Artifactory - I assume we could have some form of publication workflow similar to https://docs.github.com/en/free-pro-team@latest/actions/guides/publishing-java-packages-with-maven

The last two points are not blockers to getting this merged and have separate PRs. But it might be useful to discuss and agree on the overall progression and the goals for the upcoming release and beyond. /cc @ome/formats

@jburel
Copy link
Member Author

jburel commented Nov 18, 2020

  • i have been looking at other repositories since and I think the maven file could be turned into a template. I will need to look into the script used in more details. But we will need a maven template
  • This PR is not looking at the publishing steps. We opted to hold off for now, I need to go back to the work on sonatype.

@sbesson
Copy link
Member

sbesson commented Nov 18, 2020

This PR is not looking at the publishing steps. We opted to hold off for now, I need to go back to the work on sonatype.

Agreed we held off on the full transition. So you would keep Travis CI -> mvn deploy -> OME Artifactory for now and investigate the GH workflow -> mvn deploy -> OSSRH as a follow-up? Or would there be some advantage into looking into GH workflow-> mvn deploy -> OME Artifactory as an intermediate step?

@jburel
Copy link
Member Author

jburel commented Nov 18, 2020

our version of artifactory is not the most recent. I remember having to rewrite some code when working on Gradle due to the version of artifactory. So the same problem might happen with GH actions

@jburel
Copy link
Member Author

jburel commented Nov 18, 2020

I will remove the maven file from this PR. Creating a template workflow

@jburel
Copy link
Member Author

jburel commented Nov 18, 2020

maven.yml will be replaced by the template file introduced in ome/.github#14

@jburel
Copy link
Member Author

jburel commented Dec 8, 2020

@dgault dgault added this to the 6.6.0 milestone Dec 9, 2020
@dgault
Copy link
Member

dgault commented Dec 9, 2020

In short, happy to get this merged now as it gets us where we need to be without causing any breaking changes. The issue with the use of Location(String, String) versus new Location(String) should no longer be a problem and I don't see it having an impact in this scenario. I will likely open a follow up PR to further clean up the DynamicMetadataOptions code though and improve the providers in the unit tests.

@dgault dgault merged commit a5f4a24 into ome:develop Dec 9, 2020
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.

4 participants