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

tc-build: Add initial scripts and update README #1

Closed
wants to merge 5 commits into from

Conversation

nathanchance
Copy link
Member

Review away (and add reviewers as necessary)

Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
@nickdesaulniers
Copy link
Member

probably won't be able to provide a quick review (this week or next), hopefully we can wait for that.

@nathanchance
Copy link
Member Author

That's fine, that will also give time for others to test and review.

@ojeda
Copy link
Member

ojeda commented Mar 28, 2019

A few quick comments:

  • Since we are downloading files here, we should verify them for the user too. Since we download only particular ones, I suggest to keep the SHA-256 hash in the repo and verify it after the curl. This avoids depending on gpg and playing with keyrings.

  • Commenting why each CMAKE_DEFINES option was added would be nice, even if it is a one-liner. I know details can be found in the various LLVM's CMakeLists.txts etc., but it takes time to re-discover the reasoning behind each (and why we added it in these scripts in particular) if you are not working all day within LLVM.

  • (Don't hate me with this one :) In cases like this, I always suggest to use a proper language for this stuff (e.g. Python), even if it introduces a dependency. These sh scripts seem already complex enough to warrant it, but I know it is a pain to rewrite now that you are done. Python's argparse in particular is very nice for this, and also avoids -usage.txt files etc.

@nathanchance
Copy link
Member Author

nathanchance commented Mar 28, 2019

Since we are downloading files here, we should verify them for the user too.

Great idea, done: nathanchance@29eeff9

Commenting why each CMAKE_DEFINES option was added would be nice, even if it is a one-liner.

I agree: nathanchance@2065cac

I know it is a pain to rewrite now that you are done

Also not to mention that I don't know Python :) but yes, I suppose I should probably learn it so that stuff like this can be more readable to others.

@nishanthkarthik
Copy link

@nathanchance I have some experience with python. If we list the project roadmap, I can contribute too!

@msfjarvis
Copy link
Member

@nathanchance I have some experience with python. If we list the project roadmap, I can contribute too!

I can help with that 👍

@ojeda
Copy link
Member

ojeda commented Mar 28, 2019

@nathanchance That was quick! :)

Great idea, done: nathanchance@29eeff9

Thanks a lot! I am not sure what you mean by:

This avoids gpg shenanigans even though it'd be more secure.

Checking signatures is useful because it is a way to verify any possible future new files (or hashes) signed by the same people. However, in this case we have a known file (which you verified its authenticity using the signature before computing the hash), so unless SHA-256's second preimage resistance is broken, I don't see how signatures add anything here.

I agree: nathanchance@2065cac

Great! Some are obvious, of course, but the ones that aren't are very useful. Quick note on LLVM_ENABLE_BINDINGS: there are more than OCaml bindings, no? (Python and Go AFAICS in llvm/bindings). Maybe copy-paste error?

Also not to mention that I don't know Python :) but yes, I suppose I should probably learn it so that stuff like this can be more readable to others.

I didn't mean shell scripts are unreadable [*], but that as soon as shell scripts get complex enough (loops, testing, handling lists, etc.), it is usually nicer to do this stuff in Python (or the like) instead, in my opinion. I had to maintain both shell scripts and Python scripts for similar stuff in the past and nowadays I basically never write any shell scripts anymore unless they are shorter than 10 lines!

Python 100% recommended if you have the time to take a look at it. But don't worry about rewriting this, of course, unless somebody feels up for it.

[*] They are, though ;D

@nathanchance
Copy link
Member Author

@nathanchance That was quick! :)

Great idea, done: nathanchance@29eeff9

Thanks a lot! I am not sure what you mean by:

This avoids gpg shenanigans even though it'd be more secure.

Checking signatures is useful because it is a way to verify any possible future new files (or hashes) signed by the same people. However, in this case we have a known file (which you verified its authenticity using the signature before computing the hash), so unless SHA-256's second preimage resistance is broken, I don't see how signatures add anything here.

Yes, you're right, I'll remove that line.

I agree: nathanchance@2065cac

Great! Some are obvious, of course, but the ones that aren't are very useful. Quick note on LLVM_ENABLE_BINDINGS: there are more than OCaml bindings, no? (Python and Go AFAICS in llvm/bindings). Maybe copy-paste error?

Yes (although I never actually looked into the bindings folder).

Also not to mention that I don't know Python :) but yes, I suppose I should probably learn it so that stuff like this can be more readable to others.

I didn't mean shell scripts are unreadable [*], but that as soon as shell scripts get complex enough (loops, testing, handling lists, etc.), it is usually nicer to do this stuff in Python (or the like) instead, in my opinion. I had to maintain both shell scripts and Python scripts for similar stuff in the past and nowadays I basically never write any shell scripts anymore unless they are shorter than 10 lines!

Python 100% recommended if you have the time to take a look at it. But don't worry about rewriting this, of course, unless somebody feels up for it.

[*] They are, though ;D

I won't necessarily mind if someone rewrites this but at the same time, I won't feel comfortable maintaining it.

So that users can be fairly confident the binutils package is legitimate without needing gpg.

Suggested-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Suggested-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
SC1117: Backslash is literal in "\0". Prefer explicit escaping: "\\0".
SC2028: echo won't expand escape sequences. Consider printf.

All false positives because of our use of 'echo -e'.

Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
@E5ten
Copy link

E5ten commented Mar 30, 2019

I'd say that even if someone who knew python was willing to rewrite it, in my opinion shell scripts are significantly better for this, they get the job done without adding a dependency, and the build they're automating would be done using commands in a shell if not automated, so it's much easier to understand when the automation is done in a shell script instead of python.

@ojeda
Copy link
Member

ojeda commented Mar 30, 2019

and the build they're automating would be done using commands in a shell if not automated, so it's much easier to understand when the automation is done in a shell script instead of python.

That holds true if your shell script consisted only of a series of commands to be executed sequentially, the same way you did in the shell manually. But as soon as your shell script starts making decisions, takes parameters/flags, you document those parameters, etc. then you are not doing what you did in the terminal. Instead, it begins to look like an actual program, and for writing actual programs, there are better choices out there.

@nickdesaulniers
Copy link
Member

Also not to mention that I don't know Python :) but yes, I suppose I should probably learn it

Having a smallish project that you're interested in is always a great opportunity to learn a new language.

@nathanchance
Copy link
Member Author

Having a smallish project that you're interested in is always a great opportunity to learn a new language.

Fair point, I'll see if I can find some time soon to read up about it and see how complicated it would be to rewrite these.

@ojeda
Copy link
Member

ojeda commented Apr 12, 2019

If you need any help, don't be shy to contact! If you end doing it, don't get overwhelmed by Python's huge standard library. To do this kind of stuff, the most important ones you will want to use are:

  • subprocess for calling processes (and handling their return codes, output, piping, etc. etc.).
  • argparse for building a full CLI, including sub-commands a la Git.
  • pathlib for handling paths properly.
  • shutil for the usual filesystem operations.
  • logging is very useful even on its basic form to avoid doing raw prints and to aid on debugging.

There are other related modules, but those tend to be either lower-level or older equivalents (typically with less features or more awkward to use). As a general guideline, try to avoid the os module in particular if you can do it with one of the others, as well as the getopt one.

@nathanchance
Copy link
Member Author

nathanchance commented Apr 16, 2019

I am currently rewriting this in Python, progress can be tracked here: https://github.com/nathanchance/tc-build/commits/python-bringup

Please feel free to review as I go if you see something that can be improved. I hope to have a v2 pull request by tonight but we'll see how testing goes :)

Closing this now.

@nickdesaulniers
Copy link
Member

Looks great so far! @gwelymernans wrote a python linter; if you really want the hardcore "google3 python readability" review, maybe he can spare some cycles for it? (surviving "google3 c++ readability" made me a much much much better C++ programmer).

@nickdesaulniers
Copy link
Member

The README could be flushed out. It should address questions like:

  1. what is this repo?
  2. why does it exist?
  3. how do I run it?

Needs a LICENSE file.

@nathanchance
Copy link
Member Author

@gwelymernans wrote a python linter; if you really want the hardcore "google3 python readability" review, maybe he can spare some cycles for it?

Please, I was planning on linting it once I was done writing it.

The README could be flushed out. Needs a LICENSE file.

Copy. Those two will come from this pull and I'll tidy them up.

@bwendling
Copy link

The reformatter is at https://github.com/google/yapf :-)

@jyizheng jyizheng mentioned this pull request Sep 10, 2020
ElectroPerf pushed a commit to Atom-X-Devs/atom-x-tc-build that referenced this pull request Mar 10, 2023
Update ohmyzsh installation command

Change-Id: I5fd8131433e59bb2ff6760e68c76c11152fb0c3c
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.

7 participants