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

Lca/age symstore #25

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions nimp/base_commands/upload.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,11 @@ def configure_arguments(self, env, parser):
'--compress',
help = 'Compress symbols when uploading',
action = 'store_true')
parser.add_argument('--age-symstore',
default = False,
type = int,
help = 'Age store, in days')
# action = 'store_true')

return True

Expand All @@ -62,6 +67,11 @@ def is_available(self, env):

def run(self, env):
success = True
if env.age_symstore:
if not nimp.build.age_symstore(env):
success = False
return success
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@jasugun jasugun Mar 10, 2022

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

Both sounds fine to me. Whichever you prefer.

Copy link
Contributor Author

@jasugun jasugun Mar 10, 2022

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@jasugun jasugun Mar 10, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, being in build doesn't make much sense.


# ps5 shipping has no corresponding symbols, they're in the elf/self unstripped binary file
has_symbols_inside_binary_file = env.platform in ['ps5']
for config_or_target in env.configurations:
Expand Down
69 changes: 59 additions & 10 deletions nimp/build.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@
import re
import time

from datetime import datetime, timedelta

import nimp.system
import nimp.sys.platform
import nimp.sys.process
Expand Down Expand Up @@ -274,12 +276,10 @@ def install_distcc_and_ccache():
logging.debug('Setting UBT_PARALLEL=%s', workers)
os.environ['UBT_PARALLEL'] = workers

def upload_symbols(env, symbols, config):
''' Uploads build symbols to a symbol server '''
if not (env.is_win64 or env.is_xsx or env.is_ps5):
logging.error("Plafrom must be win64, xsx or ps5")
return False
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):
Copy link
Member

Choose a reason for hiding this comment

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

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?

def _discover_latest_autosdk(platform):
'''look for autoSDK on buildmachine '''
# TODO: get this out of here and in a more generic place like system for example
Expand Down Expand Up @@ -313,24 +313,73 @@ def _discover_latest_autosdk(platform):
return auto_sdk_root

# 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)
Copy link
Member

Choose a reason for hiding this comment

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

I think this store_root logic is not in the correct function anymore.

if not os.path.exists(store_root):
nimp.system.safe_makedirs(store_root)

# 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'
Copy link
Member

Choose a reason for hiding this comment

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

These two lines would be better in the else L330

if env.is_ps5: # ps5 sym tool
auto_sdk_root = _discover_latest_autosdk(env.platform)
prospero_local_root = os.getenv('SCE_PROSPERO_SDK_DIR', default=None)
assert prospero_local_root or auto_sdk_root
ps5_sdk_root = auto_sdk_root if auto_sdk_root else prospero_local_root
sym_tool_path = os.path.join(ps5_sdk_root, 'host_tools', 'bin', 'prospero-symupload.exe')
else: # autoSDK win10 sdk
win10_sdk_path = 'D:/autoSDK/HostWin64/Win64/Windows Kits/10/Debuggers/x64/symstore.exe'
win10_sdk_path = f'D:/autoSDK/HostWin64/Win64/Windows Kits/10/Debuggers/x64/{win64_tool}.exe'
if os.path.isfile(win10_sdk_path):
sym_tool_path = win10_sdk_path
sym_tool_path = nimp.system.sanitize_path(sym_tool_path)
logging.debug('Using sym-too-path -> %s' % sym_tool_path)

logging.debug('Using sym-too-path -> %s' % symstore_tool_path)
return sym_tool_path

def has_symstoring(env):
if not (env.is_win64 or env.is_xsx or env.is_ps5):
logging.error("Plafrom must be win64, xsx or ps5")
return False
return True

def age_symstore(env):
if not has_symstoring(env):
return False

store_root = get_symstore_root(env)
symstore_tools_path = get_symstore_tool_path(env, agestore=True)
cmd = [ symstore_tools_path ]

if env.is_ps5:
before_date = (datetime.today() - timedelta(days=env.age_symstore)).strftime("%Y-%m-%d %H:%M:%S")
cmd += [
"cleanup",
"/s", store_root, # target symbol store
"/o", # Verbose output
f'/before "{before_date}"', # number of days not supported before sdk 4.0+, we have to use dates
"/preview" if env.dry_run else "", # preview if dry-run
]
elif env.is_microsoft_platform:
cmd += [
store_root, # target symbol store
"-s", # recursive
"-y", # no confirmation prompt
f"-days={env.age_symstore}",
"-l" if env.dry_run else "", # preview if dry-run
]
else:
logging.error("Platform is not supported")
return False

if nimp.sys.process.call(cmd) != 0:
return False
return True

def upload_symbols(env, symbols, config):
''' Uploads build symbols to a symbol server '''
if not has_symstoring(env):
return False

symstore_tool_path = get_symstore_tool_path(env)

compress_symbols = hasattr(env, 'compress') and env.compress
compression_type = None
Expand All @@ -356,7 +405,7 @@ def _discover_latest_autosdk(platform):

# common cmd params
cmd = [
sym_tool_path,
symstore_tool_path,
"add",
"/r", # Recursive
"/f", "@" + index_file, # add files from response file
Expand Down