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

Support BreakingGround-DLC in instance faking #2773

Merged
merged 2 commits into from
Jun 27, 2019

Conversation

DasSkelett
Copy link
Member

@DasSkelett DasSkelett commented Jun 6, 2019

Got a bit bigger than I thought.
(And it's getting bigger and bigger)

Changes

Important stuff first:
Console command for faking changed!
It is now ckan ksp fake <name> <path> <version> [--MakingHistory <version>] [--BreakingGround <version>]
instead of previous ckan ksp fake <name> <path> <version> [MakingHistoryVersion].
I think this is more future-proof.
I am going to try my luck adjusting KSP-CKAN/xKAN-meta_testing.

IDlcDetector:
I added string InstallPath(), it returns the install path relative to the GameDir, that means for example "GameData/SquadExpansion/Serenity".
Depending on how this idea of mine will be implemented (if at all), we might restructure the whole thing again. But I think this is the best solution for now, I'm open for feedback.

I added bool AllowedOnBaseVersion(KspVersion), it returns whether a DLC is allowed to be installed or faked on a given basegame version.

I added

string IdentifierBaseName { get; }
KspVersion ReleaseGameVersion { get; }

to allow accessing those two from outside the interface implementations.
I changed the two fields with these names inside StandardDlcDetectorBase to getter without setter (this effectively is like the readonly modificator before).

CloneFakeInstanceDialog:
The GUI CloneFakeInstanceDialog has been adjusted:
CloneFakeInstanceDialog
If Squad releases another 10 DLCs we might get out of space and need a new system, but for now it works.

FakeInstance():
FakeInstance() is now a transaction.
Before the DLC just wasn't faked if f.e. the KSP version was < 1.4.0.
This left the user in an unfavorable situation, he had to delete the new folder and run ckan ksp forget once and try again to include the DLC. now it's all or nothing.

FakeInstance() takes the DLCs that should be faked as Dictionary<IDlcDetector, KspVersion>. Future DLC releases shouldn't need a change in this fuction.

Also CopyDirectory() is now a real transaction that includes IO operations, if I understand stock .NET transactions right they don't include them, but using ChinhDo.Transactions with TxFileManager should fix this.
Correct me if I got something wrong there (or anywhere else too of course).

Fixes #2791

Oh yeah, #2749 needs another rebase ^^

@DasSkelett DasSkelett added Enhancement New features or functionality GUI Issues affecting the interactive GUI Cmdline Issues affecting the command line Core (ckan.dll) Issues affecting the core part of CKAN Pull request labels Jun 6, 2019
@DasSkelett
Copy link
Member Author

DasSkelett commented Jun 6, 2019

I see that the "bug" that DLCs weren't faked without error message was used in the validation scripts.
@techman83, you could link the xkcd again...

But I still believe that it should behave the new way.

We can use the ckan compare command to determine whether to fake a DLC or not in the scripts.
Right now it outputs a very human readable answer:
"1.1.0" is lower than "1.4.1".
but if we add another option (--machine-readable or so) to output -1, 0 or 1 it should be more usable.

@HebaruSan

This comment has been minimized.

DasSkelett added a commit to DasSkelett/xKAN-meta_testing that referenced this pull request Jun 6, 2019
Also adjust the `ckan ksp fake` syntax for
KSP-CKAN/CKAN#2773
DasSkelett added a commit to DasSkelett/xKAN-meta_testing that referenced this pull request Jun 6, 2019
@DasSkelett
Copy link
Member Author

The scripts have a function versions_less_or_equal to do version comparisons.

That's cool, thanks!

@HebaruSan

This comment has been minimized.

@DasSkelett
Copy link
Member Author

DasSkelett commented Jun 11, 2019

The shoe is on the other foot!

Darn it, feared this would happen ;)
Will take some days until I'm home again.

@HebaruSan

This comment has been minimized.

@DasSkelett
Copy link
Member Author

'Merged' the PR, thanks.

Cmdline/Action/KSP.cs Outdated Show resolved Hide resolved
Cmdline/Action/KSP.cs Outdated Show resolved Hide resolved
Core/KSPManager.cs Outdated Show resolved Hide resolved
Core/KSP.cs Outdated Show resolved Hide resolved
@HebaruSan

This comment has been minimized.

Core/KSP.cs Outdated Show resolved Hide resolved
@HebaruSan

This comment has been minimized.

@DasSkelett
Copy link
Member Author

Another minor point, the name error ends with "--Done--" while the folder error ends with "--Error--". Looks like this is because Manager.HasInstance(installName) will return true thanks to the instance that's already using the name?

It's because I just forgot to put a return in that specific kraken catch, so the execution continues and yes, Manager.HasInstance() finally returns true.

* Redo `ckan ksp fake` command syntax.

* Adjust `CloneFakeKspDialog`

* Extend IDlcDetector with `InstallPath()` and `AllowedOnBaseVersion()`.

* Make `FakeInstance()` a transaction, cover IO operations in `CopyDirectory()` transaction,
    make `KSP.SetupCkanDirectories()` transaction aware and add bool argument to
    skip directory scanning.
@DasSkelett
Copy link
Member Author

Okay, some more changes:

  • Added new kraken: WrongKspVersionKraken, to be used if a given KSP version is valid, but not allowed for an action (like instance faking and older base game version).
  • Renamed IncorrectKspVersionKraken to BadKspVersionKraken to better distinct it from the new one. To be used if a version is not valid, i.e. not in the build map or malformatted.
  • Added another test to check if FakeInstance() throws WrongKspVersionkraken for base game versions that are too old for the DLC(s) requested to fake.
  • Redid the exit code handling in Cmdline.KSP.FakeNewInstance() a bit.

@DasSkelett
Copy link
Member Author

Does the bot skip network-dependent tests?

@HebaruSan
Copy link
Member

I believe so:

- ./build test+only --configuration=$BUILD_CONFIGURATION --where="cat!=FlakyNetwork"

@DasSkelett
Copy link
Member Author

Yep, just discovered that flag. Was wondering, because on my machine I obviously fail the two Curseforge tests.

@HebaruSan HebaruSan merged commit 688e4ca into KSP-CKAN:master Jun 27, 2019
@DasSkelett DasSkelett deleted the feature/fake-more-dlcs branch June 27, 2019 17:22
@DasSkelett DasSkelett added the i18n Issues regarding the internationalization of CKAN and PRs that need translating label Jan 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cmdline Issues affecting the command line Core (ckan.dll) Issues affecting the core part of CKAN Enhancement New features or functionality GUI Issues affecting the interactive GUI i18n Issues regarding the internationalization of CKAN and PRs that need translating
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Out-of-order checks during Fake creation
3 participants