Skip to content

fix nested cloning issue; add some installation of build essential and cmake; update readme#146

Merged
5iri merged 5 commits intoSRA-VJTI:mainfrom
badboy1606:fix-install-script
Mar 8, 2026
Merged

fix nested cloning issue; add some installation of build essential and cmake; update readme#146
5iri merged 5 commits intoSRA-VJTI:mainfrom
badboy1606:fix-install-script

Conversation

@badboy1606
Copy link
Copy Markdown
Contributor

Fix install script issues

This PR fixes issues in install.sh:

  • Removes nested repository cloning
  • Prevents directory confusion during installation
  • Fixes permission errors when modifying /etc/localtime
  • Makes installer safe to run from repo root

Tested on Pop!_OS(24.04 LTS) .

@crisiumnih
Copy link
Copy Markdown

In clean Ubuntu install git is not installed
This script serves the purpose to install every requirement and then clone pixels
A better alternative will be to check if pixels is already cloned (mostly in home directory) if not then clone

If the repository is already cloned running make install is enough and there’s no need to run install.sh.
An alternative (and cleaner) approach would be to improve the Makefile to also install build tools (pkg-config, toolchain, make). That would give two clear paths:

  1. install.sh -> fresh setup
  2. make install -> dependency setup only

@5iri confirm

@5iri
Copy link
Copy Markdown
Member

5iri commented Jan 29, 2026

all good, your idea make sense @badboy1606, do it this way.

it also allows for people who have git clone'd it earlier to run make install or just make in the $PROJECT_ROOT and it should build tools accordingly.

so the process in which I am thinking is,

a person does wget to download install.sh and this shell script build the essential tools and then hands over to make install for Pixels specific installations.

does this makes sense? @crisiumnih

@badboy1606
Copy link
Copy Markdown
Contributor Author

So I have updated the install.sh.
Now all the dependencies will be installed using the Makefile and not the install.sh script

can you please review @crisiumnih @5iri

install.sh Outdated
}

# Detect OS
#OS detection
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.

why are you changing this comment? doesn't show any significance though?

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.

ok i will do it as it is

Makefile Outdated

@echo "installing missing dependencies..."

ifneq (,$(findstring ubuntu,$(OS)))
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.

either you reduce the loc or increase it for formatting? you have increased it here and reduced it on

   debian|wsl) sudo apt install -y make ;;

@badboy1606
Copy link
Copy Markdown
Contributor Author

badboy1606 commented Feb 24, 2026

switched to the original makefile only and just added some installation of buiild essential and cmake

@5iri 5iri changed the title fixed nested cloning issue while installation and updated readme fix nested cloning issue; add some installation of buiild essential and cmake; update readme Feb 25, 2026
@5iri 5iri changed the title fix nested cloning issue; add some installation of buiild essential and cmake; update readme fix nested cloning issue; add some installation of build essential and cmake; update readme Feb 25, 2026
Copy link
Copy Markdown
Member

@5iri 5iri left a comment

Choose a reason for hiding this comment

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

lgtm

@5iri
Copy link
Copy Markdown
Member

5iri commented Mar 6, 2026

@crisiumnih can you also check once?

@5iri 5iri merged commit 8b0c9c2 into SRA-VJTI:main Mar 8, 2026
5 checks passed
@badboy1606 badboy1606 deleted the fix-install-script branch March 8, 2026 13:34
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.

3 participants