Lca/age symstore - #25
Conversation
This takes the numbers of days we want to keep (through files modification times).
| if env.age_symstore: | ||
| if not nimp.build.age_symstore(env): | ||
| success = False | ||
| return success |
There was a problem hiding this comment.
providing the --age-symstore arg will completely change this command behaviour.
I think it'd be better to have a separate command for this rather than integrating it as such.
There was a problem hiding this comment.
Do you mean outside of the nimp upload command?
To be fair, we could have a more specfic command for this whole symstoring thing like nimp symstore with subcommands:
nimp symstore upload
nimp symostore clean
Or do you mean adding a subcommand to the existing like nimp upload cleanup?
There was a problem hiding this comment.
Both sounds fine to me. Whichever you prefer.
There was a problem hiding this comment.
I think I'd rather rewrite the command as symstore:
- upload sounds awfuly vague to me (and we already have upload-fileset)
- looks like we only use this in one place in buildbot in _update_symbol_server() so not a hassle to follow up changes
There was a problem hiding this comment.
Moreover, don't you think the upload / symstore related stuff in nimp.build could moved into the symstore command itself?
I don't think they serve any purpose outside of symstoring.
There was a problem hiding this comment.
I agree, being in build doesn't make much sense.
| def get_symstore_root(env): | ||
| return nimp.system.sanitize_path(os.path.join(env.format(env.publish_symbols), env.platform.lower())) | ||
|
|
||
| def get_symstore_tool_path(env, agestore=False): |
There was a problem hiding this comment.
Really not a fan of this new arg here.
Maybe splitting these functions would help?
Like
def get_symstore_tool_path
and get_agestore_tool_path
which both would be implement as
def func():
if env.is_ps5:
return get_ps5_symupload_tool_path()
elif microsoft:
return get_win_tool_path([agestore | symstore])
What do you think?
|
|
||
| # create store if not available yet | ||
| store_root = nimp.system.sanitize_path(os.path.join(env.format(env.publish_symbols), env.platform.lower())) | ||
| store_root = get_symstore_root(env) |
There was a problem hiding this comment.
I think this store_root logic is not in the correct function anymore.
| # find the tool to upload our symbols | ||
| sym_tool_path = "C:/Program Files (x86)/Windows Kits/10/Debuggers/x64/symstore.exe" | ||
| win64_tool = 'agestore' if agestore else 'symstore' | ||
| sym_tool_path = f'C:/Program Files (x86)/Windows Kits/10/Debuggers/x64/{win64_tool}.exe' |
There was a problem hiding this comment.
These two lines would be better in the else L330
|
Wrongly closed by dev branch deletion |
2 participants
No description provided.