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 tide framework #5862

Merged
merged 10 commits into from
Nov 18, 2022

Conversation

krishnaTORQUE
Copy link
Contributor

No description provided.

@waghanza
Copy link
Collaborator

When running inside the container, I have

/usr/src/app/target/release/server: /lib/x86_64-linux-gnu/libm.so.6: version `GLIBC_2.29' not found (required by /usr/src/app/target/release/server)

@krishnaTORQUE

cc @msrd0

@krishnaTORQUE
Copy link
Contributor Author

@waghanza
I think you need build-base package.
If you are using alpine then here is apk add build-base

@waghanza
Copy link
Collaborator

@krishnaTORQUE
Copy link
Contributor Author

you mean https://packages.debian.org/fr/buster/build-essential, I think

Yes maybe for debian. What I just see in dockerfile.
But it suppose to be there on build time & not runtime.
2nd is rust image should have this as well. (Not sure)

@waghanza
Copy link
Collaborator

seems that rust base image does not have those libs installed. the dockerfile is

FROM rust:1.65-slim AS build
WORKDIR /usr/src/app
COPY . ./
RUN cargo build --release && strip target/release/server
FROM debian:buster-slim
COPY --from=build /usr/src/app/target/release/server /usr/src/app/target/release/server
CMD /usr/src/app/target/release/server

@krishnaTORQUE
Copy link
Contributor Author

@waghanza
Try this & let me know

FROM rust:1.65-slim AS build
WORKDIR /usr/src/app
COPY . ./
RUN apt install libc6-dev build-essential # <-- Here Try it & let me know
RUN cargo build --release && strip target/release/server
FROM debian:buster-slim
COPY --from=build /usr/src/app/target/release/server /usr/src/app/target/release/server
CMD /usr/src/app/target/release/server

@waghanza
Copy link
Collaborator

same with

FROM rust:1.65-slim AS build

WORKDIR /usr/src/app

RUN apt-get -qy update

  RUN apt-get -qy install libc6-dev
  RUN apt-get -qy install build-essential

COPY . ./

RUN cargo build --release && strip target/release/server

FROM debian:buster-slim

COPY --from=build /usr/src/app/target/release/server /usr/src/app/target/release/server

CMD /usr/src/app/target/release/server

@msrd0
Copy link
Contributor

msrd0 commented Nov 16, 2022

This is probably due to the debian version mismatch. You're building with the default debian-based rust docker image which should be based on debian bullseye but run it in debian buster. Try FROM debian:slim for the 2nd stage to use the latest debian version, or pin the debian version in the first stage.

@waghanza
Copy link
Collaborator

@jbr @Fishrock123 @yoshuawuyts @skade do you consent to include tide here ? beside, if any one of you allow me to notify him/her on each (minor) update, it will be 👍🏻

@jbr
Copy link

jbr commented Nov 16, 2022

In the past tide's preference was to not participate in benchmarks when there was an option, which is part of why the TechEmpower framework benchmark entry was written by a third party and is unofficial. Tide is currently not actively being developed, but I'm not sure if that is reason to exclude it from your benchmark.

@waghanza
Copy link
Collaborator

As I understand above thread @jbr, tide is :

  • still maintained (at least not because of feature addition, but because somehow some folks could patch it in case)
  • used in production

for this reason, I think it could be included here (I'll be glad if some folks help me on maintaining the implementation here). also it could be a great way to detect failures (compilation issue with the last rust version ....)

but if any main contributor does not want it to be included here, I'll respect this choice ❤️

@krishnaTORQUE
Copy link
Contributor Author

As I understand above thread @jbr, tide is :

* still maintained (at least not because of feature addition, but because somehow some folks could patch it in case)

* used in **production**

for this reason, I think it could be included here (I'll be glad if some folks help me on maintaining the implementation here). also it could be a great way to detect failures (compilation issue with the last rust version ....)

but if any main contributor does not want it to be included here, I'll respect this choice heart

I can maintain tide as I am also maintaining the warp. I have no issue.
But yes, if author does not want it, then we should respect their choice.

@krishnaTORQUE
Copy link
Contributor Author

@waghanza
I am going to close this PR for now. & Raise another PR after I test & fix the issue with docker in my system.
Is it fine?

@waghanza
Copy link
Collaborator

I prefer to keep it open for a while (some days probably), just to make sure we gather consent (or non-opposition) of author(s)

@krishnaTORQUE
Copy link
Contributor Author

I prefer to keep it open for a while (some days probably), just to make sure we gather consent (or non-opposition) of author(s)

Yes sure,
That make sense

@msrd0
Copy link
Contributor

msrd0 commented Nov 17, 2022

Regardless of tide getting added to the benchmark or not, you should keep the changes you made to the Dockerfile.

@waghanza
Copy link
Collaborator

@msrd0 changes are already in master. after digging into docker hub, we need to use debian:stable-slim instead of debian:slim, and better to have this for all rust frameworks

@waghanza waghanza enabled auto-merge (squash) November 18, 2022 11:18
@waghanza waghanza merged commit e321fdc into the-benchmarker:master Nov 18, 2022
@krishnaTORQUE krishnaTORQUE deleted the added-tide-framework branch November 18, 2022 11:32
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.

None yet

4 participants