-
Notifications
You must be signed in to change notification settings - Fork 9
package: use CustomConfig instead of DNEConfigVariant #95
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
base: main
Are you sure you want to change the base?
Conversation
nimp/base_commands/package.py
Outdated
| # necessary for shader debug info in case no defaultEngine is present | ||
| _setup_default_config_file(f'{variant_configuration_directory}/DefaultEngine.ini') | ||
| _setup_default_config_file(f'{variant_configuration_directory}/DefaultGame.ini') | ||
| Package.write_project_revisions(env, variant_configuration_directory) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the DneVariant flow:
- We copy the variant to an avtive variant folder, that will get all the edits in write_project_revisions
- This works because the active variant folder is not perforced in any case.
In the CustomConfig flow:
- We're working with a perfoced custom folder right? In this case I think the edits won't work out of the box (it might have worked because you tested stuff in your monorepo with no files locked by perforce).
- You might want it peforce edit/revert wrap the file in that case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we'd have the custom config folders on Perforce. I was thinking we'd have them on Git, like we do for variants currently
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Package are create Perforce side, after a sync from Git to Perforce, so the context of this command is a Perforce client
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jasugun I think this subject is the last thing we need to deal with for this to be mergeable. I see two solutions:
- add
p4 edit/revertcalls before modifying the files and after the commandlet is done - replicate the "copy into an
Activedirectory" pipeline that we have for variants. We already have code in place that loads the CustomConfig from theActivedirectory (we need it to have the config applied in packages and not just during the cook process), so this could work pretty much out of the box
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried the second approach with this commit, seems to work properly on my end 83ca0f1
nimp/base_commands/package.py
Outdated
| Package.write_project_revisions(env, active_configuration_directory) | ||
| _setup_default_config_file(f'{configuration_directory}/DefaultEngine.ini') | ||
| _setup_default_config_file(f'{configuration_directory}/DefaultGame.ini') | ||
| Package.write_project_revisions(env, configuration_directory) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the DNEVariant workflow, we're editing an active folder that's not perforced.
The CustomConfig folder will be perforced I think, so we'll need to perfroce edit/revert wrap before editing in this case.
More generally, beware that we're not editing something from the CustomConfig folder somewhere else in the packaging code without the perforce wrap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate of #95 (comment)?
9f29193 to
2c55941
Compare
|
Wrongly closed by dev branch deletion |
Co-authored-by: Thomas Desveaux <[email protected]>
2c55941 to
e1a8656
Compare
No description provided.