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

Reduce Global Variable Assignment 01 #1615

Closed
wants to merge 5 commits into from
Closed

Reduce Global Variable Assignment 01 #1615

wants to merge 5 commits into from

Conversation

zpiatt
Copy link
Contributor

@zpiatt zpiatt commented Aug 24, 2023

I got a lot more work done on reducing global variables. I made some assumptions for variable/function names that will need to be reviewed.

  1. Removed all system calls to basename opting for parameter expansions instead.
  2. Localized variables that were already in functions, but were not required globally.
  3. Created new functions to facilitate localizing variables.
    • The apply_patches function now iterates across the patches explicitly using find rather than relying on globbing. This also allows the test [[ -f ]] to be removed and prevents any errors white-space could introduce.
  4. Moved variables that were defined outside of a function, inside the function that was calling them.
  5. Attempted to clarify when variables were meant to be global/local.
    • This is inconsistent, but it appears the intent was to make global variables all caps. Where applicable I made local variables lowercase and/or camel-case per previous style guidance. If this style was consistent it would make it a lot easier to further reduce global variables and optimize the build process further.

As always, I'm happy to make any tweaks you require.

@zpiatt
Copy link
Contributor Author

zpiatt commented Aug 24, 2023

I see there's an error with the find command in apply_patches. I mistakenly added a trailing /. It also occurs to me -maxdepth should be set so find doesn't enter the insider directory unless the conditional is passed.

@zpiatt
Copy link
Contributor Author

zpiatt commented Aug 24, 2023

Is it possible there's an issue with /patches/fix-build-linux.patch. I'm not seeing an obvious reason this should be failing.

@zpiatt
Copy link
Contributor Author

zpiatt commented Aug 24, 2023

It looks like Run ./build.sh has been failing the last few days. At various patches it fails as below:

failed to apply patch ../patches/fix-build-linux.patch
Error: Process completed with exit code 1.

@daiyam
Copy link
Member

daiyam commented Aug 24, 2023

@zpiatt It's due to #1608...

@zpiatt
Copy link
Contributor Author

zpiatt commented Aug 24, 2023

@zpiatt It's due to #1608...

That's what I thought. It also failed at the same step (though on a different patch) before that was merged: https://github.com/VSCodium/vscodium/actions/runs/5948672438/job/16132919035#step:9:76

@zpiatt zpiatt closed this Sep 21, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants