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

deps/docker: add 'time' as a dependency for tests #5933

Merged
merged 1 commit into from
May 3, 2024

Conversation

trws
Copy link
Member

@trws trws commented May 3, 2024

problem: Apparently bookworm, jammy, focal, and new fedoras don't install /usr/bin/time by default. I ran into this trying to include time output in a test.

Evidently the "time" builtin in bash is suppressed inside the scope of an eval like we use in sharness, either way this seems less annoying than having to avoid it.

solution: install it, added to each of the docker files as well as the install-deps-*.sh files.

@trws trws requested a review from grondo May 3, 2024 16:41
Copy link
Contributor

@grondo grondo left a comment

Choose a reason for hiding this comment

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

Were you adding a test requiring /usr/bin/time in flux-core? If not, any reason this shouldn't go in the flux-sched Dockerfiles like the other flux-sched specific deps?

Anyway, it is pretty trivial so probably no reason to not include it here, since it might be surprising that it is missing..

Did you already rebuild and push up the affected fluxrm/testenv images?b

Thanks!

@trws
Copy link
Member Author

trws commented May 3, 2024

I have not pushed them yet, and no it wasn't in core, honestly the thought was more "why on earth would time not work in eval?!?" since I would expect the bash builtin to work even if it wasn't there. Having tripped over it now 3 or 4 times I guessed others might and it was cheap enough to fix.

If you're happy with this I'll start the run to push up the new testenv images momentarily, just give me an ACK.

@trws
Copy link
Member Author

trws commented May 3, 2024

Hm, actually, since I'm in here, should I update the fedora dockerfile too? We're currently running 38 I think when current is 40, and 40 needs this same tweak (it's what I'm running on my dev VM).

@grondo
Copy link
Contributor

grondo commented May 3, 2024

Feel free to update to fedora40 though that process is usually a bit time consuming if you want to hold off.

problem: Apparently bookworm, jammy, focal, and new fedoras don't
install /usr/bin/time by default.  I ran into this trying to include
time output in a test.

solution: install it, added to each of the docker files as well as the
install-deps-*.sh files.
@trws trws force-pushed the ask-me-for-anything-but-time branch from 8604445 to 1d749bb Compare May 3, 2024 20:59
@trws
Copy link
Member Author

trws commented May 3, 2024

Ok, added time to fedora39 so when that gets updated we don't have to remember. Waiting for the last two aarch64 container builds to finish (emulated, so they're kinda slow), as soon as that happens and they're cleanly posted I'll MWP this.

Copy link

codecov bot commented May 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.36%. Comparing base (e38a0cd) to head (1d749bb).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5933      +/-   ##
==========================================
- Coverage   83.38%   83.36%   -0.02%     
==========================================
  Files         514      514              
  Lines       83055    83055              
==========================================
- Hits        69253    69240      -13     
- Misses      13802    13815      +13     

see 9 files with indirect coverage changes

@mergify mergify bot merged commit 135d8af into flux-framework:master May 3, 2024
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants