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

Escape and wrap compileIr task argument for Windows #597

Merged
merged 3 commits into from
Sep 22, 2020

Conversation

vcatalano
Copy link
Contributor

Before this PR

When running the compileIr task on Windows systems, we receive an error due to the extensions argument of the command not being properly wrapped and escaped for the Windows .bat command. The stack trace for this issue can be found here: #594

After this PR

Windows systems will be able to successfully compile Conjure products.
Update compileIr task to handle escaping command line arguments for Windows

Possible downsides?

I'm not sure the compileConjure task has been working properly with Windows for a while. Cloning the most recent Conjure Java Example project (https://github.com/palantir/conjure-java-example) fails to build on Windows. I've tested this on numerous machines so it seems to be specific to the Conjure project.

@palantirtech
Copy link
Member

Thanks for your interest in palantir/gradle-conjure, @vcatalano! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

@changelog-app
Copy link

changelog-app bot commented Sep 17, 2020

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Escape and wrap compileIr task argument for Windows

Before this PR

When running the compileIr task on Windows systems, we receive an error due to the extensions argument of the command not being properly wrapped and escaped for the Windows .bat command. The stack trace for this issue can be found here: #594

After this PR

Windows systems will be able to successfully compile Conjure products.
Update compileIr task to handle escaping command line arguments for Windows

Possible downsides?

I'm not sure the compileConjure task has been working properly with Windows for a while. Cloning the most recent Conjure Java Example project (https://github.com/palantir/conjure-java-example) fails to build on Windows. I've tested this on numerous machines so it seems to be specific to the Conjure project.

Check the box to generate changelog(s)

  • Generate changelog entry

@ferozco ferozco closed this Sep 17, 2020
@ferozco ferozco reopened this Sep 17, 2020
@ferozco
Copy link
Contributor

ferozco commented Sep 17, 2020

Thanks for the contribution @vcatalano! We don't have CI for windows environments so could you please confirm that the change you've made works as intended? You should be able to do so by publishing locally by running CIRCLE_TAG=dev ./gradlew pTML and then picking it up in your project by adding mavenLocal() as a buildscript repository and setting the version of gradle-conjure to dev

@vcatalano
Copy link
Contributor Author

@ferozco I was able to build the version locally and point our project to the local maven repository. I can confirm these changes now allow us to build our project on Windows environments!

@vcatalano
Copy link
Contributor Author

@ferozco Let me know if there is anything else you need in order to verify the functionality for Windows. Our team is trying to use Conjure for one of our projects but we are blocked by the developers that are using Windows devices. Getting a new release with these changes would help us out greatly.

Copy link
Contributor

@iamdanfox iamdanfox left a comment

Choose a reason for hiding this comment

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

Approving as Felipe is OOTO this week, thanks for the contribution @vcatalano

Just wanna add the caveat that since we don't use windows dev machines at Palantir, I'd caution that you may end up finding other Windows-related papercuts I'm afraid.

@bulldozer-bot bulldozer-bot bot merged commit 640a78d into palantir:develop Sep 22, 2020
@svc-autorelease
Copy link
Collaborator

Failed to load project - please reach out to #dev-foundry-infra or check Aries to debug

@iamdanfox
Copy link
Contributor

iamdanfox commented Sep 22, 2020

Top marks autorelease... I'll cut one manually. nvm it's an easy fix: #602, https://github.com/palantir/gradle-conjure/releases/tag/4.27.1 should be up soon!

@vcatalano
Copy link
Contributor Author

Awesome, thanks for the help @iamdanfox! I really appreciate this project!

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.

None yet

5 participants