Skip to content

'sed -i' need postfix on mac#3312

Closed
DaanHoogland wants to merge 2 commits into
apache:masterfrom
DaanHoogland:apidocToolingOnMac
Closed

'sed -i' need postfix on mac#3312
DaanHoogland wants to merge 2 commits into
apache:masterfrom
DaanHoogland:apidocToolingOnMac

Conversation

@DaanHoogland

Copy link
Copy Markdown
Contributor

Description

build fails on mac due to #3247 . adding extension that will only be created in '/target/' anyway

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)

Screenshots (if appropriate):

How Has This Been Tested?

@DaanHoogland

Copy link
Copy Markdown
Contributor Author

ping @GabrielBrascher @nathanejohnson , this slipped in end of march. didn't notice, too much on fedora :(, not enough on a mac :)

sed -e 's,%API_HEADER%,All APIs,g' "$thisdir/generatetoc_header.xsl" >generatetoc.xsl
sed -i "s/%ACS_RELEASE%/${ACS_RELEASE}/g" generatetoc.xsl
sed -i "s/%ACS_RELEASE%/${ACS_RELEASE}/g" generatecommands.xsl
sed -i .bak "s/%ACS_RELEASE%/${ACS_RELEASE}/g" generatetoc.xsl

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What if we execute the commands at line 63 and 64 like the one in 62? Or, maybe the other way around.

I mean, they do the same thing, but with different approaches. Therefore, it might be interesting to maintain some consistency here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@rafaelweingartner, not entirely sure what you mean but 62 an 63 can certainly be combined. not sure how 64 can, it is touching a different file.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I meant the following:
All of the commands are replacing "place holders" in the same file. However, two of them are doing in line (via -i flag), whereas line 62 is replacing the entire file (not using the -i flag and redirecting the output of the sed to a file with the same name). I do not mind if we do in different commands the replace, but we could at least try to use the same approach in all of them.

By approach here I mean the method, either inline or not.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think you are misreading the code,
line 62 redirects to a new file, from "$thisdir/generatetoc_header.xsl" to generatetoc.xsl
then 63 operates on the resulting generatetoc.xsl
but 64 is operating on generatecommands.xsl

I don't think shell operations would allow redecting to input, it would empty the file before openening it for read if you tried that.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@DaanHoogland @rafaelweingartner A (ugly) solution is running the command with the -e and move the new file to the old one:
sed -e "s/%ACS_RELEASE%/${ACS_RELEASE}/g" newGeneratetoc.xsl && mv newGeneratetoc.xsl generatetoc.xsl;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@DaanHoogland on linux the -i must be appended with the .bak (-i.bak)

-i[SUFFIX], --in-place[=SUFFIX]
              edit files in place (makes backup if SUFFIX supplied)

@yadvr

yadvr commented May 16, 2019

Copy link
Copy Markdown
Member

@DaanHoogland env issue, the sed on OSX is freebsd based you can try gnu-sed like:

$ brew install gnu-sed

@DaanHoogland

Copy link
Copy Markdown
Contributor Author

@DaanHoogland env issue, the sed on OSX is freebsd based

I realize that, but it complicates build and thus inhibits people entering the playing field. I would prefer adding the extension, it is in the target dir and doesn't hurt anybody.

@nathanejohnson

Copy link
Copy Markdown
Member

@rhtyd you can easily add gnu sed to mac via homebrew as you point out, however the binary is called 'gsed' to avoid conflicting with the main sed installed in /usr/bin .

@nathanejohnson nathanejohnson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@yadvr

yadvr commented May 23, 2019

Copy link
Copy Markdown
Member

@blueorangutan package

@yadvr yadvr added this to the 4.13.0.0 milestone May 23, 2019
@blueorangutan

Copy link
Copy Markdown

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan

Copy link
Copy Markdown

Packaging result: ✖centos6 ✖centos7 ✖debian. JID-2752

@GabrielBrascher

Copy link
Copy Markdown
Member

@DaanHoogland I don't think that this would work on linux. The -i must be appended with the .bak (-i.bak)

Documentation:

-i[SUFFIX], --in-place[=SUFFIX]
              edit files in place (makes backup if SUFFIX supplied)

@yadvr yadvr closed this May 27, 2019
@yadvr yadvr reopened this May 27, 2019
@yadvr

yadvr commented May 27, 2019

Copy link
Copy Markdown
Member

closed-open PR to rekick Travis
@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@DaanHoogland

Copy link
Copy Markdown
Contributor Author

@DaanHoogland I don't think that this would work on linux. The -i must be appended with the .bak (-i.bak)
@GaborApatiNagy, I'll try and find time to test this on fedora and if so I'll checnge it to the long option.
I hope there is some compatible option/argument set between the two.

@blueorangutan

Copy link
Copy Markdown

Packaging result: ✖centos6 ✖centos7 ✖debian. JID-2799

@DaanHoogland

Copy link
Copy Markdown
Contributor Author

travis is quite clear in this case: needs tlc

@yadvr

yadvr commented May 27, 2019

Copy link
Copy Markdown
Member

@DaanHoogland can you debug via the backend jenkins what failed?

@DaanHoogland

Copy link
Copy Markdown
Contributor Author

@rhtyd if you mean travis, yes. but kind of high prio -3.

@yadvr

yadvr commented Jun 20, 2019

Copy link
Copy Markdown
Member

@DaanHoogland fails on linux with:

Warning, API Cmd class com.cloud.api.commands.SimulatorAddSecondaryAgent has no APICommand annotation 
Scanned and found 607 APIs
sed: -e expression #1, char 1: unknown command: `.'
[ERROR] Failed to execute goal org.codehaus.mojo:exec-maven-plugin:1.2.1:exec (compile) on project cloud-apidoc: Command execution failed. Process exited with an error: 1 (Exit value: 1) -> [Help 1]

@yadvr yadvr removed this from the 4.13.0.0 milestone Jun 25, 2019
@yadvr

yadvr commented Jul 3, 2019

Copy link
Copy Markdown
Member

This is onhold until Travis passes.

@DaanHoogland DaanHoogland mentioned this pull request Mar 24, 2020
5 tasks
@DaanHoogland DaanHoogland marked this pull request as draft May 7, 2020 08:33
@soreana soreana mentioned this pull request Aug 8, 2020
5 tasks
yadvr pushed a commit that referenced this pull request Aug 10, 2020
apidoc build failed in Mac OS because of sed in-place command. It is a minor change that fixed this issue. More information here

Fixes: #3247
Fixes: #3312
@yadvr

yadvr commented Aug 10, 2020

Copy link
Copy Markdown
Member

Closing as #4253 fixed it

@yadvr yadvr closed this Aug 10, 2020
@DaanHoogland DaanHoogland deleted the apidocToolingOnMac branch August 16, 2023 11:30
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.

8 participants