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

Unhardcode mod package loaders #13223

Merged
merged 5 commits into from May 12, 2017

Conversation

Projects
None yet
4 participants
@pchote
Member

pchote commented May 1, 2017

This updates #13171 with architectural fixes and addresses the other FileSystem hacks that were blocked on having mod-defined package loaders.

This breaks the cycling dependency between assemblies that define package loaders that contain assemblies that define package loaders that... by requiring all dlls to be loaded from a normal directory. This then also fixes @abcdefg30's SharpDevelop problems.

This is already a monster of a PR, so i'm going to defer moving all the package loaders to the mod assemblies to a future PR. I've done the D2K sound parser as a testcase, but moving any more would make it difficult to keep a clean git history while addressing any review comments.

The final commit shows that it is still possible to explicitly mount subdirectories inside zip files.

I strongly suggest reviewing this commit by commit, using the ?w=1 parameter to squash whitespace changes on the main package loader commits.

@pchote pchote added this to the Next Release milestone May 1, 2017

@pchote pchote requested review from RoosterDragon and abcdefg30 May 1, 2017

@abcdefg30

Works, thanks! I can't review the code 100%, but it looked okay as well.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote May 7, 2017

Member

Fixed, rebased, and dropped the test commit.

Member

pchote commented May 7, 2017

Fixed, rebased, and dropped the test commit.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 7, 2017

Contributor

and dropped the test commit.

That was a bit early, could you push a separate test branch? Because I can't get it to work locally, but I'm probably just making a mistake somewhere.

Contributor

reaperrr commented May 7, 2017

and dropped the test commit.

That was a bit early, could you push a separate test branch? Because I can't get it to work locally, but I'm probably just making a mistake somewhere.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 7, 2017

Contributor

I can get ra|bits.zip to work but not ra|bits.zip/desert, neither on bleed nor here, though.

Contributor

reaperrr commented May 7, 2017

I can get ra|bits.zip to work but not ra|bits.zip/desert, neither on bleed nor here, though.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote May 7, 2017

Member

You need to do ra|bits.zip: bits and then bits|desert.

Member

pchote commented May 7, 2017

You need to do ra|bits.zip: bits and then bits|desert.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote May 7, 2017

Member

That was a bit early, could you push a separate test branch?

pchote@26eddcf

Member

pchote commented May 7, 2017

That was a bit early, could you push a separate test branch?

pchote@26eddcf

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 7, 2017

Contributor

Test case works 👍

I'd still prefer either @RoosterDragon, @atlimit8 or someone else who is better than me at reviewing code to look over it before this is merged, so I won't merge this right away.

Contributor

reaperrr commented May 7, 2017

Test case works 👍

I'd still prefer either @RoosterDragon, @atlimit8 or someone else who is better than me at reviewing code to look over it before this is merged, so I won't merge this right away.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 12, 2017

Contributor

I gave the code another look since nobody else seems to have the time or motivation, couldn't find anything that looks wrong, and in addition to the testcase all mods still seem to work fine. I assume that if there is any bug lurking that we overlooked, we'll probably run into it before the next major release, so I'll merge this now.

Contributor

reaperrr commented May 12, 2017

I gave the code another look since nobody else seems to have the time or motivation, couldn't find anything that looks wrong, and in addition to the testcase all mods still seem to work fine. I assume that if there is any bug lurking that we overlooked, we'll probably run into it before the next major release, so I'll merge this now.

@reaperrr reaperrr merged commit 5e73652 into OpenRA:bleed May 12, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment