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

debian: fix Built-Using parse error with Jaeger #38744

Closed
wants to merge 1 commit into from

Conversation

clwluvw
Copy link
Contributor

@clwluvw clwluvw commented Jan 1, 2021

Built-Using won't be parsed because of spacing

Signed-off-by: Seena Fallah seenafallah@gmail.com

@clwluvw
Copy link
Contributor Author

clwluvw commented Jan 4, 2021

@ideepika @tchaikov PTAL

@@ -107,7 +107,7 @@ Build-Depends: automake,
# Make-Check xmlstarlet,
nasm [amd64],
zlib1g-dev,
# Jaeger Built-Using: libyaml-cpp-dev (>= 0.6),
# Jaeger Built-Using: libyaml-cpp-dev (>= 0.6),
Copy link
Contributor

Choose a reason for hiding this comment

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

@clwluvw could you be more specific on "fix"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tchaikov Here https://github.com/ceph/ceph/blob/master/install-deps.sh#L68 when removing # Jaeger from this line debian/control file will be like this:

...
# Make-Check   xmlstarlet,
               nasm [amd64],
               zlib1g-dev,
      Built-Using:   libyaml-cpp-dev (>= 0.6),
...

and it's not parsable!

Copy link
Contributor

@tchaikov tchaikov Jan 5, 2021

Choose a reason for hiding this comment

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

@clwluvw i think a better way is to fix the regular expression used in install-deps.sh. for instance,

sed -i -e 's/^# Jaeger[[:space:]]\+//g' $control

@ideepika how did you test this change? i am wondering if you pushed an old branch?

Copy link
Member

Choose a reason for hiding this comment

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

@tchaikov thanks for the ping, missed the notification
@tchaikov I used centos to test these, I missed catching them.
@clwluvw we need to also remove #crimson libyaml-cpp if we are using yaml-cpp that supports jaeger, otherwise there would be build failures due to linking to older version of libyaml.
#Jaeger would also need some indentation here for debian/control to parse correctly, can you update changes considering these points, something like:
https://github.com/ceph/ceph/pull/38783/files#diff-1335d27b188d672bf9a9b572f40df7c4699b8d9998fb16ba284f4acb4e77077cR74

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ideepika The first part has been done in #38743. Could you please first review and merge that?

Copy link
Member

Choose a reason for hiding this comment

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

@clwluvw https://github.com/ceph/ceph/pull/38743/files#r580288000 and it would be ideal if you can amend all in one PR, easier to review :)

Copy link
Member

Choose a reason for hiding this comment

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

@clwluvw https://github.com/ceph/ceph/pull/38743/files#r580288000 and it would be ideal if you can amend all in one PR, easier to review :)

@tchaikov, this needs to be addressed, I couldn't get back to complete this pr: https://github.com/ceph/ceph/pull/38743/files#r580288000 will do if @clwluvw is not working on it

Copy link
Contributor

Choose a reason for hiding this comment

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

@clwluvw hi Seena, are you still working on this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tchaikov @ideepika sorry for the delay. I've made an update here: #38743

@clwluvw clwluvw force-pushed the jaeger-debian branch 2 times, most recently from c487fd9 to 727a4fc Compare January 5, 2021 14:14
@clwluvw clwluvw requested a review from tchaikov January 5, 2021 14:15
install-deps.sh Outdated Show resolved Hide resolved
Signed-off-by: Seena Fallah <seenafallah@gmail.com>
@clwluvw
Copy link
Contributor Author

clwluvw commented Jan 8, 2021

@ideepika PTAL

@tchaikov
Copy link
Contributor

@ideepika ping?

@tchaikov
Copy link
Contributor

tchaikov commented Apr 4, 2021

@ideepika ping?

@ideepika
Copy link
Member

@clwluvw closing this PR as https://github.com/ceph/ceph/pull/38743/files addresses this issue

@ideepika ideepika closed this Apr 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants