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

Added instructions on how to run all benchmarks for different operating systems #213

Merged
merged 11 commits into from
Jun 6, 2021

Conversation

fvbakel
Copy link
Contributor

@fvbakel fvbakel commented Jun 1, 2021

Description

Added instructions on how to run all benchmarks for different operating systems. This is a solution for issue #203.

Contributing requirements

Not applicable.

  • I read the contribution guidelines in CONTRIBUTING.md.
  • I placed my solution in the correct solution folder.
  • I added a README.md with the right category badge(s).
  • I added a Dockerfile that builds and runs my solution.
  • I selected drag-race as the target branch.
  • All code herein is licensed compatible with BSD-3.

@fvbakel fvbakel changed the base branch from drag-race to original June 1, 2021 14:42
@fvbakel fvbakel changed the base branch from original to drag-race June 1, 2021 14:42
@marghidanu
Copy link
Contributor

Please check the new tools implementation, the system dependencies have changed. This document needs to be updated, the benchmark tooling is still a work in progress.

BENCHMARK.md Outdated Show resolved Hide resolved
BENCHMARK.md Outdated Show resolved Hide resolved
BENCHMARK.md Outdated Show resolved Hide resolved
BENCHMARK.md Outdated Show resolved Hide resolved
BENCHMARK.md Outdated Show resolved Hide resolved
@rbergen
Copy link
Contributor

rbergen commented Jun 1, 2021

The remark that @marghidanu made just before I posted my review translates to Node.js being an additional requirement for the benchmark to run, next to make, bash, Docker (and WSL2 on Windows). This requirement only applies to the new tools setup (see PR #214), but as the new tools are likely to be merged on the short term and Node.js being installed doesn't break the current tools, I agree that it's best to add this to BENCHMARK.md from the beginning.

@fvbakel
Copy link
Contributor Author

fvbakel commented Jun 1, 2021

@rbergen Thanks for your review. I applied the comments. I don't know the details about PR #214, but I hope it doesn't break this description to much :-).

@rbergen
Copy link
Contributor

rbergen commented Jun 1, 2021

I don't know the details about PR #214, but I hope it doesn't break this description to much :-).

It shouldn't. Like I said, it basically adds Node.js (and npm) as an additional prerequisite for the benchmark to run. On Windows, it is important that the nodejs and npm packages are installed inside the WSL2 Ubuntu distro (just like make), not on Windows itself.

Edit: I forgot to mention that the Node.js/npm versions that are included in many OS-provided package managers are too old. This means that it will generally be required to download and install an appropriate version of Node.js from https://nodejs.org/en/download/. Personally, I have successfully tested the new tools setup with the current LTS version (that being 14.17.0 at the time of writing).

README.md Outdated Show resolved Hide resolved
@rbergen
Copy link
Contributor

rbergen commented Jun 3, 2021

@fvbakel We have just merged the new benchmark tool (PR #214), which means the Node.js dependency now applies. Would it be possible to add the installation instructions for Node.js to your PR?

@fvbakel
Copy link
Contributor Author

fvbakel commented Jun 4, 2021

Yes, I can include the Node.js instruction somewhere next week.

@rbergen
Copy link
Contributor

rbergen commented Jun 4, 2021

That's great news and highly appreciated!

@fvbakel
Copy link
Contributor Author

fvbakel commented Jun 4, 2021

I have run the new reporting on Linux and that works fine. I also tested on Windows with WSL2, Ubuntu 20.04 and the same Linux binary installed, The solution are all run fine, but the reporting step fails. This is the error:

npm ci && npm run start -- report -d /tmp/tmp.fShEnvV3CO
/home/fvbakel/node-v14.17.0-darwin-x64/bin/node: 1: ���: not found
/home/fvbakel/node-v14.17.0-darwin-x64/bin/node: 1: cannot open ��: No such file
/home/fvbakel/node-v14.17.0-darwin-x64/bin/node: 1: ��: not found
/home/fvbakel/node-v14.17.0-darwin-x64/bin/node: 1: �� not found
/home/fvbakel/node-v14.17.0-darwin-x64/bin/node: 1: �����#: not found
/home/fvbakel/node-v14.17.0-darwin-x64/bin/node: 1: �����: not found
/home/fvbakel/node-v14.17.0-darwin-x64/bin/node: 2: __nl_symbol_ptr__DATAa�a���v�__got__DATa�a���w�__la_symbol_ptr__DATAsb��b���__mod_init_func__DATAb��b��: not found
/home/fvbakel/node-v14.17.0-darwin-x64/bin/node: 6: Syntax error: "(" unexpected

Anny idea how this can be fixed?

@rbergen
Copy link
Contributor

rbergen commented Jun 4, 2021

I'm a bit confused. Darwin is usually aimed at MacOS. How exactly did you install Node within WSL2?

@fvbakel
Copy link
Contributor Author

fvbakel commented Jun 4, 2021

:-) Indeed, by accidentally extracted the macOS binaries on the WSL2. Tested again with the Linux distribution, that works fine.

@fvbakel
Copy link
Contributor Author

fvbakel commented Jun 4, 2021

@rbergen Added the Node.js instructions.

Copy link
Contributor

@rbergen rbergen left a comment

Choose a reason for hiding this comment

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

I think the Node.js installation instructions you've added are a good addition, indeed. The one comment I'm making is basically to avoid confusion about Makefiles vs. shell scripts, for those who are not fully aware of the differences between the two.

BENCHMARK.md Outdated Show resolved Hide resolved
Copy link
Contributor

@rbergen rbergen left a comment

Choose a reason for hiding this comment

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

LGTM

@rbergen rbergen requested a review from marghidanu June 4, 2021 17:05
BENCHMARK.md Outdated Show resolved Hide resolved
BENCHMARK.md Outdated Show resolved Hide resolved
@rbergen rbergen merged commit d113f34 into PlummersSoftwareLLC:drag-race Jun 6, 2021
@fvbakel fvbakel deleted the issues/203 branch June 8, 2021 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants