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.

Sign up for GitHub

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

Jump to bottom

Compatibility/feature upgrades for playtest-20201213. #170

Merged
merged 3 commits into from Jan 30, 2021
Merged

Compatibility/feature upgrades for playtest-20201213. #170

merged 3 commits into from Jan 30, 2021

Conversation

pchote
Copy link
Member

@pchote pchote commented Nov 28, 2020

Brings compatibility up to current bleed. Keeping as a draft until upstream changes have been completed and we have an engine tag available.

@pchote pchote force-pushed the engine-updates branch 18 times, most recently from 0df9964 to 8e1e9d8 Compare December 2, 2020 00:09
@pchote pchote mentioned this pull request Dec 2, 2020
Merged
@pchote pchote force-pushed the engine-updates branch 8 times, most recently from 9cbfc83 to 77726f6 Compare December 5, 2020 19:51
@pchote
Copy link
Member Author

pchote commented Dec 5, 2020

Test builds: https://github.com/pchote/OpenRAModSDK/releases/tag/devtest-20201205
CI log: https://github.com/pchote/OpenRAModSDK/actions/runs/402974502
Packaging log: https://github.com/pchote/OpenRAModSDK/actions/runs/402977546

This PR should now hopefully be complete, but I will keep it as a draft until the upstream PRs have been merged.

@pchote pchote force-pushed the engine-updates branch 2 times, most recently from 9de6ce9 to 073daba Compare December 12, 2020 00:14
@pchote
Copy link
Member Author

pchote commented Jan 9, 2021

Thanks for the thorough review! Updated.

Mailaender
Mailaender reviewed Jan 9, 2021
View reviewed changes
Copy link
Member

@Mailaender Mailaender left a comment

Choose a reason for hiding this comment

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

I verified these work on @OpenHV.

@pchote
Copy link
Member Author

pchote commented Jan 11, 2021 •
edited

I have discovered a couple of small omissions here while working on the next stage (.NET5 compatibility). I will push fixes later.

  • make.ps1 clean references stale paths
  • make.ps1 references old utility path and obsolete OpenRA.StyleCheck.exe
  • macos packaging can remove entitlements.plist
  • make check isn't passing TARGETPLATFORM

@pchote pchote force-pushed the engine-updates branch 3 times, most recently from 0df8d26 to 3b2c46d Compare January 11, 2021 22:30
@pchote
Copy link
Member Author

pchote commented Jan 11, 2021

Fixed.

@pchote
Copy link
Member Author

pchote commented Jan 14, 2021

Updated. make.ps1 now chains to the engine's check command and ported the fix from OpenRA/OpenRA#18963.

abcdefg30
abcdefg30 reviewed Jan 15, 2021
View reviewed changes
make.ps1 Outdated
Comment on lines 382 to 383
# Run the same command on the engine's make file
if ($command -eq "all" -or $command -eq "clean" -or $command -eq "check")
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look correct. If the engine exists and we take at line 319 we'll run the command twice, won't we?

Copy link
Member Author

@pchote pchote Jan 19, 2021 •
edited

Choose a reason for hiding this comment

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

I honestly don't remember even making this change, so I can't explain my motivation for doing this. I assume this is trying to work around a bug.

Why are we only fetching the engine only on all or clean to begin with? Surely we should always be checking for and fetching the engine, and then the individual *-Commands should be forcing a recompile if required, like the main makefile does.

Copy link
Member Author

@pchote pchote Jan 24, 2021 •
edited

Choose a reason for hiding this comment

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

Updated to remove this change.

I think this was trying to work around the error messages from rm when you try to clean an already clean build. I have instead changed the SDK make.ps1 to use Remove-Item -ErrorAction Ignore, and filed OpenRA/OpenRA#19085 to make the same fix in the engine.

@pchote pchote mentioned this pull request Jan 24, 2021
Merged
@pchote pchote requested a review from abcdefg30 January 30, 2021 11:34
abcdefg30
abcdefg30 reviewed Jan 30, 2021
View reviewed changes
make.ps1 Outdated Show resolved Hide resolved
@abcdefg30 abcdefg30 mentioned this pull request Jan 30, 2021
Merged
@pchote
Copy link
Member Author

pchote commented Jan 30, 2021

Added a new commit to remove the windows docs target.

@pchote
Update .gitignore
a8afa90 
* Remove obsolete entries
* Add .idea (JetBrains Rider cache)
@pchote
Copy link
Member Author

pchote commented Jan 30, 2021

Added another new commit as discussed on IRC to update the .gitignore.

abcdefg30
abcdefg30 approved these changes Jan 30, 2021
View reviewed changes
Copy link
Member

@abcdefg30 abcdefg30 left a comment

Choose a reason for hiding this comment

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

Lgtm now.

@abcdefg30 abcdefg30 merged commit 017d8bf into OpenRA:master Jan 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mailaender Mailaender Mailaender left review comments

@abcdefg30 abcdefg30 abcdefg30 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@pchote @Mailaender @abcdefg30