Skip to content

Add working Linux tests #81

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

Merged
merged 8 commits into from
Jul 17, 2022
Merged

Conversation

adam-fowler
Copy link
Contributor

As with Graphiti, GraphQL does not have working Linux tests.
This PR resolves this.

@adam-fowler
Copy link
Contributor Author

The thread sanitizer is complaining.

@adam-fowler adam-fowler marked this pull request as draft May 12, 2021 13:31
@adam-fowler adam-fowler marked this pull request as ready for review May 12, 2021 16:11
@adam-fowler
Copy link
Contributor Author

Removed the thread sanitizer. Issues are with Runtime package and cannot be resolved here. I've added an issue wickwirew/Runtime#87

@adam-fowler
Copy link
Contributor Author

Is there any reason GraphQL doesn't use Mirror instead of the Runtime package?

@paulofaria
Copy link
Member

Back then, Mirror wasn't powerful enough! Do you reckon it is possible to replace Runtime with Mirror? I haven't heard of any updates regarding reflection in Swift. I'd love to simplify the codebase and remove the dependency, if possible.

@adam-fowler
Copy link
Contributor Author

Back then, Mirror wasn't powerful enough! Do you reckon it is possible to replace Runtime with Mirror? I haven't heard of any updates regarding reflection in Swift. I'd love to simplify the codebase and remove the dependency, if possible.

All you seem to be using it for is extracting values out of a type using the variable name. I'll add a PR to demonstrate

@paulofaria
Copy link
Member

@adam-fowler I think updating from master should solve the issue with GH actions.

@paulofaria
Copy link
Member

Basically adding the macos-latest?

@adam-fowler
Copy link
Contributor Author

@paulofaria This change will need a change to the required tests.

The ubuntu tests you had before didn't test anything. This is why I added this PR. But the change uses the swift docker images which have different names to the ubuntu images required at the moment. The swift docker images are better to use because we can be specific about which version of swift we want running.

As an aside isn't macos-10.15 the same as macos-latest?

@paulofaria
Copy link
Member

As an aside isn't macos-10.15 the same as macos-latest?

@adam-fowler Yeah, I think so. @alexsteinerde might chime in and let us know why adding "macos-latest" fixes the GH actions stall issue. If changing to "macos-latest" and removing macos-10.15 works, I'm cool with just using "macos-latest".

@NeedleInAJayStack
Copy link
Member

@adam-fowler With this change, should we remove the ubuntu items from this line in the existing workflow yml?

@adam-fowler
Copy link
Contributor Author

Looks like it needs updated to support 5.5 and 5.6. Although since I posted the new swift install actions are available which could simplify this more. I'll rebuild this using them.

Since Swift 5.4 it is not a requirement
It appears the codeclimate-action messes with the swift setup
@adam-fowler
Copy link
Contributor Author

Can we get rid of the required tests Build and test on macos-10.15 and Build and test on ubuntu-18.04. @NeedleInAJayStack don't know if you have the power. Maybe @paulofaria can do this.

@paulofaria paulofaria closed this Jul 17, 2022
@paulofaria
Copy link
Member

Added @NeedleInAJayStack to the admins team. He should have full access now.

@paulofaria paulofaria reopened this Jul 17, 2022
@paulofaria
Copy link
Member

Closed by accident.

@paulofaria
Copy link
Member

Hm, not sure how I can get rid of that? Maybe we should force merge, then that requirement will be gone, since it won't be in the yml file anymore? This looks like a GH bug, I reckon.

@paulofaria paulofaria merged commit edd1d67 into GraphQLSwift:master Jul 17, 2022
@adam-fowler
Copy link
Contributor Author

Hm, not sure how I can get rid of that? Maybe we should force merge, then that requirement will be gone, since it won't be in the yml file anymore? This looks like a GH bug, I reckon.

It is in the settings for the project (something only admins can access). Do you have branch protection setup?

@NeedleInAJayStack
Copy link
Member

I've removed Build and test on macos-10.15 and Build and test on ubuntu-18.04 from the master branch protection rules

@NeedleInAJayStack
Copy link
Member

NeedleInAJayStack commented Jul 17, 2022

Scratch that - I've changed the branch protection rules to be the actions that ran when this was merged to master. Specifically, the actions required are:

  • Build and test on macos-latest
  • Build 5.4 on ubuntu-latest
  • Build 5.5 on ubuntu-latest
  • Build 5.6 on ubuntu-latest

Thanks for this work @adam-fowler!

@adam-fowler
Copy link
Contributor Author

I think it's pretty pointless adding required builds. You already require an admin approval. Also the required builds will change overtime as new versions of swift are released.

Swift NIO is only ever planning to support the last three versions of swift so there is no real point for a project reliant of NIO trying to support more version of swift than that. So when 5.7 is released support for 5.4 won't be required. Of course you can still support 5.4 if you want.

@NeedleInAJayStack
Copy link
Member

I agree with you Adam - requiring code owner approval seems sufficient to me. I'll let @paulofaria make the final call on this one though since he's been the primary maintainer.

@paulofaria
Copy link
Member

Yeah! Let's do it!

@NeedleInAJayStack
Copy link
Member

Awesome, I've removed the required status checks from GraphQL and aligned the projects throughout GraphQLSwift to have the same main branch protection settings. Thanks guys!

@paulofaria
Copy link
Member

Thank you, Jay!

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