Skip to content

build-from-scratch: various cleanups#206

Merged
lgirdwood merged 1 commit intothesofproject:masterfrom
marc-hb:build-from-scratch-cleanups
Apr 3, 2020
Merged

build-from-scratch: various cleanups#206
lgirdwood merged 1 commit intothesofproject:masterfrom
marc-hb:build-from-scratch-cleanups

Conversation

@marc-hb
Copy link
Collaborator

@marc-hb marc-hb commented Mar 25, 2020

  • Fix some typos, rephrase a number of sentences.

  • Explain how to fix an out of date config-* file.

  • Move LD_LIBRARY_PATH=alsa-lib into the ALSA section as opposed to
    interrupting some unrelated flow.

  • Rename some section headers, add some in the toolchains section.

  • Add numerous ``monospace`` backticks.

  • Add missing space '#comments'

  • Use VERBOSE=1 and make help in some examples - much easier to ignore
    or remove than to guess.

  • PATH is already exported (otherwise it would not work).

  • Replace deprecated backticks `pwd` with $(pwd).
    http://mywiki.wooledge.org/BashFAQ/082

  • Replace `pwd`/../../etc with $XTENSA_ROOT which is shorter to type and
    compatible with make -C or ninja -C

  • Convert one example to cmake -B && make -C to show the possibility.

Not done to keep this large patch somewhat reviewable in a
reasonable time: address the creeping platforms duplication.

Signed-off-by: Marc Herbert marc.herbert@intel.com

@marc-hb marc-hb marked this pull request as ready for review March 25, 2020 05:27
@marc-hb
Copy link
Collaborator Author

marc-hb commented Mar 25, 2020

@lgirdwood unlike in sof, I can't add reviewers (like @xiulipan) in soc-docs, is this on purpose?

git clone https://github.com/thesofproject/crosstool-ng
cd crosstool-ng
cd ../crosstool-ng
git checkout sof-gcc8.1
Copy link
Contributor

Choose a reason for hiding this comment

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

we are in gcc9x now
https://github.com/thesofproject/crosstool-ng/tree/sof-gcc9x
@lgirdwood the Docker image is still using gcc8.1 branch like here, do we need to make the update?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As this PR is already very large, I want to draw the following line: fixes but no "upgrade". These can be tested and done in a later PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any way to "inline" or link (better) original scripts from the sof repo, in case the content is duplicated to reduce further maintenance of the copy?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mmaka1 I can add http:// links on the script names mentioned here, that's a good idea thanks. On the other hand I don't understand what duplication you're referring to, example?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mmaka1 I tried https://www.sphinx-doc.org/en/master/usage/extensions/extlinks.html and it Just Worked, pretty cool. It doesn't work inside .. code-block:: but the texts before can be rephrased to add the links. I can do this in a later PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mmaka1 I tried https://www.sphinx-doc.org/en/master/usage/extensions/extlinks.html and it Just Worked, pretty cool. It doesn't work inside .. code-block:: but the texts before can be rephrased to add the links. I can do this in a later PR.

Submitted in PR #213

make help # lists all available targets
make baytrail_defconfig
make bin -j4
make bin -j4 VERBOSE=1
Copy link
Contributor

Choose a reason for hiding this comment

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

can we make the verbose to be optional?

Copy link
Collaborator Author

@marc-hb marc-hb Mar 25, 2020

Choose a reason for hiding this comment

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

This just an example, so anyone can leave it out.
On the other hand, someone not familiar with cmake would not be able to guess it.
See commit message.

Copy link
Collaborator Author

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

Thanks @xiulipan!

git clone https://github.com/thesofproject/crosstool-ng
cd crosstool-ng
cd ../crosstool-ng
git checkout sof-gcc8.1
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As this PR is already very large, I want to draw the following line: fixes but no "upgrade". These can be tested and done in a later PR.

make help # lists all available targets
make baytrail_defconfig
make bin -j4
make bin -j4 VERBOSE=1
Copy link
Collaborator Author

@marc-hb marc-hb Mar 25, 2020

Choose a reason for hiding this comment

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

This just an example, so anyone can leave it out.
On the other hand, someone not familiar with cmake would not be able to guess it.
See commit message.

@lgirdwood
Copy link
Member

@lgirdwood unlike in sof, I can't add reviewers (like @xiulipan) in soc-docs, is this on purpose?

Nope, no one ever got around to it, you could add a code owners here to make this easier ?

Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

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

Good to catch all these.

@marc-hb
Copy link
Collaborator Author

marc-hb commented Mar 25, 2020

you could add a code owners here to make this easier ?

I can if someone tells me what its content should be (which may take longer than just doing it?)

unlike in sof, I can't add reviewers

I've never been a github admin but I didn't expect a CODEOWNERS file to help with that, seemed like a permissions issue rather.

@lgirdwood
Copy link
Member

@marc-hb permissions updated.

Copy link
Contributor

@ranj063 ranj063 left a comment

Choose a reason for hiding this comment

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

looks good @marc-hb. I must admit I didnt follow the steps as I have my build env set up already, so I'm going to take your word for it until someone else complains :)

@marc-hb marc-hb force-pushed the build-from-scratch-cleanups branch from ffe8fd2 to 4dc28d0 Compare March 27, 2020 05:05
@marc-hb
Copy link
Collaborator Author

marc-hb commented Mar 27, 2020

I must admit I didnt follow the steps as I have my build env set up already, so I'm going to take your word for it until someone else complains :)

While I thoroughly tested every change I still need people to review the style :-)

cmake ..
make -j4
mkdir build_tools
cmake -B build_tools
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't use -B of cmake's cli, because it's not in our minimum supported version (3.10).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Instead of replacing, I added -B as an example with a 3.13 version caveat. 3.13 is 1+ year old now and -B is very convenient so worth knowing.

@marc-hb marc-hb force-pushed the build-from-scratch-cleanups branch from 4dc28d0 to 8ded658 Compare March 28, 2020 19:21
@marc-hb marc-hb requested a review from jajanusz March 28, 2020 19:24
@marc-hb marc-hb force-pushed the build-from-scratch-cleanups branch from 8ded658 to 4db92cf Compare March 30, 2020 16:35
@marc-hb marc-hb requested review from xiulipan and removed request for xiulipan March 31, 2020 18:41
@marc-hb marc-hb requested review from jajanusz and removed request for jajanusz March 31, 2020 18:42
@deb-intel
Copy link
Collaborator

@jajanusz @xiulipan Have your requested changes been made?

@marc-hb marc-hb force-pushed the build-from-scratch-cleanups branch from 4db92cf to 72be34d Compare April 3, 2020 00:20
@lgirdwood
Copy link
Member

@deb-intel I think we are good to merge. @marc-hb can you fix the conflicts thanks.

- Fix some typos, rephrase a number of sentences.

- Explain how to fix an out of date config-* file.

- Move LD_LIBRARY_PATH=alsa-lib into the ALSA section as opposed to
  interrupting some unrelated flow.

- Rename some section headers, add some in the toolchains section.

- Add numerous ``monospace`` backticks.

- Add missing space '#comments'

- Use VERBOSE=1 and make help in some examples - much easier to ignore
  or remove than to guess.

- PATH is already exported (otherwise it would not work).

- Replace deprecated backticks `pwd` with $(pwd).
  http://mywiki.wooledge.org/BashFAQ/082

- Replace `pwd`/../../etc with $XTENSA_ROOT which is shorter to type and
  compatible with make -C or ninja -C

- Convert one example to cmake -B && make -C to show the possibility.

Not done to keep this large patch somewhat reviewable in a
reasonable time: address the creeping platforms duplication.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
@marc-hb marc-hb force-pushed the build-from-scratch-cleanups branch from 72be34d to f062bef Compare April 3, 2020 18:34
@marc-hb
Copy link
Collaborator Author

marc-hb commented Apr 3, 2020

@deb-intel I think we are good to merge. @marc-hb can you fix the conflicts thanks.

Done, thanks! Rebasing and fixing conflicts is not fun and exactly why I've been resisting very good suggestions to make this PR bigger than it already is (too much).

By the way there seems to be a recent increase in the number of warnings.

I just found and submitted #211 to fix the missing |JSL|.

sof-docs/algos/demux/demux.rst:62: WARNING: Pygments lexer name 'm4' is not known
sof-docs/algos/demux/demux.rst:119: WARNING: Pygments lexer name 'm4' is not known

... was just introduced by PR #203

There seems to be a couple other warnings new since I started this PR. Apparently none because of this PR.

@lgirdwood
Copy link
Member

Lets merge this now and fix the minor parts incrementally.

@lgirdwood lgirdwood merged commit 3816d70 into thesofproject:master Apr 3, 2020
@marc-hb marc-hb deleted the build-from-scratch-cleanups branch April 8, 2020 00:23
@marc-hb
Copy link
Collaborator Author

marc-hb commented Apr 8, 2020

By the way there seems to be a recent increase in the number of warnings.

I just submitted thesofproject/sof#2741 to report doxygen warnings where they happen: in sof not in sof-docs

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.

9 participants