Skip to content

Limit log file name length to OS allowed limit #76

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Limit log file name length to OS allowed limit #76

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4693200
Limit log file name length to OS allowed limit
Jun 23, 2020
ce97ee7
Attempt creating log file before trimming
Jul 16, 2020
8ed9e96
Add test for log file trimming
Jul 16, 2020
3728b54
Cleanup logname stripping comments
Jul 17, 2020
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
33 changes: 29 additions & 4 deletions gtest_parallel.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,14 +204,39 @@ def _logname(output_dir, test_binary, test_name, execution_number):
# Store logs to temporary files if there is no output_dir.
if output_dir is None:
(log_handle, log_name) = tempfile.mkstemp(prefix='gtest_parallel_',
suffix=".log")
suffix='.log')
os.close(log_handle)
return log_name

log_name = '%s-%s-%d.log' % (Task._normalize(os.path.basename(test_binary)),
Task._normalize(test_name), execution_number)
# The log file name is a combination of a test binary name and a test name.
# If the log file name exceeds OS limitations it will be truncated.
MKSTEMP_RANDOM_LEN = 12 # length of mkstemp random string being added
Copy link
Contributor

@pbos pbos Jul 27, 2020

Choose a reason for hiding this comment

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

"# Space reserved for mkstemp suffix."

MAX_PREFIX_LENGTH = 240 # Limit for file name mkstemp prefix

prefix = '%s-%s' % (Task._normalize(os.path.basename(test_binary)),
Task._normalize(test_name))
suffix = '-%d.log' % execution_number
log_name = os.path.join(output_dir, prefix + suffix)
try:
# check if log can be created or exists already
if not os.path.exists(log_name):
os.makedirs(output_dir, exist_ok=True)
with open(log_name, 'w+'):
pass
os.remove(log_name)
except OSError as os_error:
# truncate file name if error is Errno 36: File name too long
if os_error.errno == 36:
Copy link
Contributor

@pbos pbos Jul 27, 2020

Choose a reason for hiding this comment

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

Can this be errno.ENAMETOOLONG instead of the comment?

Copy link
Contributor

@pbos pbos Jul 27, 2020

Choose a reason for hiding this comment

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

https://docs.python.org/3/library/errno.html#errno.ENAMETOOLONG

prefix_length = ((os.statvfs('.').f_namemax -
MKSTEMP_RANDOM_LEN) if hasattr(os, 'statvfs') else MAX_PREFIX_LENGTH) - len(suffix)
Copy link
Contributor

@pbos pbos Jul 27, 2020

Choose a reason for hiding this comment

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

I think this would read easier as: if hasattr(..): prefix_length = ..., i.e. avoid the ternary. as it's long.

(log_handle, log_name) = tempfile.mkstemp(prefix=prefix[:prefix_length],
dir=output_dir,
suffix=suffix)
os.close(log_handle)
else:
raise
return log_name

return os.path.join(output_dir, log_name)

def run(self):
begin = time.time()
Expand Down
68 changes: 38 additions & 30 deletions gtest_parallel_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,36 +311,44 @@ def test_times_worker():

class TestTask(unittest.TestCase):
def test_log_file_names(self):
def root():
return 'C:\\' if sys.platform == 'win32' else '/'

self.assertEqual(
os.path.join('.', 'bin-Test_case-100.log'),
gtest_parallel.Task._logname('.', 'bin', 'Test.case', 100))

self.assertEqual(
os.path.join('..', 'a', 'b', 'bin-Test_case_2-1.log'),
gtest_parallel.Task._logname(os.path.join('..', 'a', 'b'),
os.path.join('..', 'bin'),
'Test.case/2', 1))

self.assertEqual(
os.path.join('..', 'a', 'b', 'bin-Test_case_2-5.log'),
gtest_parallel.Task._logname(os.path.join('..', 'a', 'b'),
os.path.join(root(), 'c', 'd', 'bin'),
'Test.case/2', 5))

self.assertEqual(
os.path.join(root(), 'a', 'b', 'bin-Instantiation_Test_case_2-3.log'),
gtest_parallel.Task._logname(os.path.join(root(), 'a', 'b'),
os.path.join('..', 'c', 'bin'),
'Instantiation/Test.case/2', 3))

self.assertEqual(
os.path.join(root(), 'a', 'b', 'bin-Test_case-1.log'),
gtest_parallel.Task._logname(os.path.join(root(), 'a', 'b'),
os.path.join(root(), 'c', 'd', 'bin'),
'Test.case', 1))
with tempfile.TemporaryDirectory() as root:
self.assertEqual(
os.path.join('.', 'bin-Test_case-100.log'),
gtest_parallel.Task._logname('.', 'bin', 'Test.case', 100))

self.assertEqual(
os.path.join('..', 'a', 'b', 'bin-Test_case_2-1.log'),
gtest_parallel.Task._logname(os.path.join('..', 'a', 'b'),
os.path.join('..', 'bin'),
'Test.case/2', 1))

self.assertEqual(
os.path.join('..', 'a', 'b', 'bin-Test_case_2-5.log'),
gtest_parallel.Task._logname(os.path.join('..', 'a', 'b'),
os.path.join(root, 'c', 'd', 'bin'),
'Test.case/2', 5))

self.assertEqual(
os.path.join(root, 'a', 'b', 'bin-Instantiation_Test_case_2-3.log'),
gtest_parallel.Task._logname(os.path.join(root, 'a', 'b'),
os.path.join('..', 'c', 'bin'),
'Instantiation/Test.case/2', 3))

self.assertEqual(
os.path.join(root, 'a', 'b', 'bin-Test_case-1.log'),
gtest_parallel.Task._logname(os.path.join(root, 'a', 'b'),
os.path.join(root, 'c', 'd', 'bin'),
'Test.case', 1))
MAX_PREFIX_LENGTH = 240
long_test_name = 'a' * (os.statvfs('.').f_namemax if hasattr(os,
'statvfs') else MAX_PREFIX_LENGTH + 1)
Copy link
Contributor

@pbos pbos Jul 27, 2020

Choose a reason for hiding this comment

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

Could you make this one a separate test? Maybe test_truncates_file_name

Also I think this test only makes sense on systems that have statvfs, can it run conditionally? This test requires OSes without statvfs to reject filenames that are longer which is maybe not what we want. I'm OK with not testing the MAX_PREFIX_LENGTH branch.

truncated_log = gtest_parallel.Task._logname(os.path.join(root, 'out'),
os.path.join(root, 'bin'),
long_test_name, 1)
self.assertNotEqual('bin-' + long_test_name + '-1.log', os.path.basename(truncated_log))
self.assertTrue(truncated_log.endswith('-1.log'))
self.assertTrue(truncated_log.startswith(os.path.join(
root, 'out', 'bin-' + long_test_name[:MAX_PREFIX_LENGTH-len('bin--1.log')])))

def test_logs_to_temporary_files_without_output_dir(self):
log_file = gtest_parallel.Task._logname(None, None, None, None)
Expand Down