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

Automatically call batch.sh commands when files re missing (mateor/auto-patcher#74) #123

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

xee5ch
Copy link
Contributor

@xee5ch xee5ch commented Jun 9, 2013

This is meant to fix #74 by doing the following:

  1. Move the batch.sh commands into a function in auto_patcher, prep_patches.
  2. In batch.sh, remove duplicating expath and abspath functions and just source auto_patcher directly.
  3. Make batch.sh just call the prep_patches function from the source auto_patcher file.

Alexander Stein added 4 commits June 8, 2013 00:52
…ith style changes in auto_patcher."

This reverts commit 2126684.
…her, and refactor to auto_patcher to not run body commands unless it is not sourced from batch.sh
…s not detected; allow legacy call from batch.sh via sourcing auto_patcher.
@mateor
Copy link
Owner

mateor commented Jun 18, 2013

Okay, I think I need to hear a little more about your thinking here...I do understand the desire to remove duplicate code (very much) but I think if the cost is an if wrapper around the entire script, we might be better off with the duplicate code at this point.

Do you think we can find a way around that wrapper? From an end-user point of view (basically my philosophy is that the ease of use and error-catching above everything else) I think that adding batch.sh if the .tgz files are not found is very beneficial...but I think we might gone overboard on how this is implemented.

Am I following your thinking right? Feel free to convince me otherwise, I am very open to discussion.

Also, maybe there is an environmental issue? This is my output from the test I ran...http://pastebin.com/Mw8Hruze

The top of that paste is a diff showing the changes of yours I have, in case I missed one. At the end is the commands I ran.

@xee5ch
Copy link
Contributor Author

xee5ch commented Jun 19, 2013

Well, the reason I wanted to implement this a certain was is my experience from Python, as it makes testing scripts, specifically those that will be library components much easier.

I even specifically looked for the if __name__ == "__main__" convention when Googling for this reason. What happens is that you can load all code in the file into another script later, but everything beneath will be executed if and only if executing the script in question. This had to be done if I take the code de-duplication route, so that if I source the auto_patcher script from batch.sh without that BASH_SOURCE block, it will start executing auto_patcher and complain how I did not give it the proper parameters, namely a ROM.

To take this further, the second reason would be for unit testing ideas you had. Unless you want to keep the unit tests in the auto_patcher file itself, it will be difficult to separate different logical units (like you have down with batch.sh and auto_patcher) and share functionality, haha dare I say functions themselves, without this method. I think the cleanliness makes it easier to code as stuff grows, but you are the boss here and this is my first FOSS adventure. So you tell me what you like.

As for your error, a quick glance makes it look like, yes, you got it right. I am not sure why the patch detection fails then. As I always try patching a ROM to test before handing off changes to you. It sounds like you do the same. Not sure what to do about that. I am really busy this week, so maybe I will take a look on the weekend if you do not mind?

@xee5ch xee5ch mentioned this pull request Jun 19, 2013
@mateor
Copy link
Owner

mateor commented Jun 20, 2013

Okay, I did a little reading on that and see more what you were thinking. I know the python syntax, but have written only very basic scripts with it. I had to Google the convention!

I just (hopefully) finished the auto-updating feature that I accidentally spammed our history with. I added a .config file to the autopatcher so that people like you and I could turn off auto-updating, as it won't play very nicely with edits to our local script or branches. I added a flag for PRODUCTION with the test suite in mind. Do you think we could use some sort of flag there? Ideally, it could be set by the command line as well, if we have to manually edit the .config file to run tests, then I think the wrapper is better.

I am closer to merging it now than I was the other day, I have a better idea of what was trying to be accomplished. Let's think on it some more as batch.sh gets fixed, etc. And there is no rush, whenever you feel the urge to tinker is just great with me.

@mateor
Copy link
Owner

mateor commented Nov 19, 2013

How about this idea: since we would be calling batch.sh from within the auto-patcher, and the auto_patcher has set -a to pass variables to children, we could just test for an auto_patcher variable within batch.sh.

  if      [[ ! "$PATCHES_VERSION" == "" ]]; then
        # batch.sh was called from auto-patcher
  fi

I am implementing something similar from a kinda toy script I am writing to automate populating and processing the autopatche patches from builds. It would be best to use something we actually need in batch.sh, like ROMX.

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.

Integrate batch.sh
2 participants