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

add tests for install & uninstall #115

Merged
merged 28 commits into from May 25, 2017
Merged

add tests for install & uninstall #115

merged 28 commits into from May 25, 2017

Conversation

ghost
Copy link

@ghost ghost commented May 23, 2017

add tests for install & uninstall, with/without sudo

I also fix the $(xmake) so that it will be always correct. See details at commit message

Have some problem on mac os on travis-ci

$(xmake) is set to join(_PROGRAM_DIR, 'xmake')
that is wrong when XMAKE_PROGRAM_DIR is set to
another program dir (like developing dir) with no executable

so use system-special ways to get absolution path
of executable file always
@waruqi
Copy link
Member

waruqi commented May 23, 2017

@titansnow Using $(xmake) has some of the following issues:

  • If the path has spaces (on windows. .e.g "C:\Program Files\xmake\xmake.exe"), double quotes are required, for example:
os.run("\"$(xmake)\" f -c")
  • If users don't see the document, they don't know if they have this variable: $(xmake), then they will still run it directly. os.run("xmake")

So, I'm going to remove $(xmake) and use os.run("xmake") directly and add the executable path of xmake to PATH env. for example:

function main()
        ..
        os.addenv("PATH", xmake._EXECUTABLE_DIR)

I think it's easier to use, and users don't have to care about the path of xmake.

to fix build on macos

`sudo luarocks install` will install to global package tree
that running as all users could find
@codecov
Copy link

codecov bot commented May 23, 2017

Codecov Report

Merging #115 into dev will increase coverage by 1.65%.
The diff coverage is 60%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #115      +/-   ##
==========================================
+ Coverage   68.13%   69.79%   +1.65%     
==========================================
  Files         246      250       +4     
  Lines       10032    10424     +392     
==========================================
+ Hits         6835     7275     +440     
+ Misses       3197     3149      -48
Impacted Files Coverage Δ
core/src/xmake/readline/readline.c 0% <0%> (ø) ⬆️
core/src/xmake/machine.c 93.75% <100%> (+7.81%) ⬆️
xmake/core/sandbox/modules/os.lua 77.58% <100%> (+1.58%) ⬆️
xmake/modules/privilege/sudo.lua 53.06% <50%> (+14.28%) ⬆️
xmake/platforms/windows/check.lua 78.85% <0%> (ø)
xmake/tools/lib.lua 96.42% <0%> (ø)
xmake/tools/link.lua 78.57% <0%> (ø)
xmake/tools/cl.lua 51.12% <0%> (ø)
xmake/core/main.lua 75.3% <0%> (+1.23%) ⬆️
xmake/actions/package/main.lua 83.82% <0%> (+1.47%) ⬆️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3768a82...67a5ad7. Read the comment docs.

@ghost
Copy link
Author

ghost commented May 23, 2017

Great! 👍

@ghost
Copy link
Author

ghost commented May 23, 2017

There is a situation that needs $(xmake). That is, sudo $(xmake). sudo will delete environment var so that the path of xmake must be known

@ghost
Copy link
Author

ghost commented May 24, 2017

luacov.runner not found on macos. Wired

@waruqi
Copy link
Member

waruqi commented May 24, 2017

@titansnow

There is a situation that needs $(xmake). That is, sudo $(xmake). sudo will delete environment var so that the path of xmake must be known.

You can use sudo -E xmake or os.sudo(os.run, "xmake f -c")(in os.lua).

Pass -E argument will preserve user environment when running sudo command. So I think $(xmake) is not necessary.

@ghost
Copy link
Author

ghost commented May 24, 2017

no use @waruqi

$ xmake l
> os.getenv("PATH")
/home/ts/.local/share/xmake:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin:/home/ts/.local/bin
> os.sudo(os.exec,"sh -c 'echo $PATH'")
[sudo] password for ts: 
/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/snap/bin
> os.exec("sudo -E sh -c 'echo $PATH'")
/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/snap/bin

@waruqi
Copy link
Member

waruqi commented May 24, 2017

@titansnow

It works on my machine(macOS and archlinux).

xmake l
> os.addenv("PATH", "/tmp/test")
> os.getenv("PATH")
/tmp/test:/Users/ruki/.cargo/bin:/Users/ruki/.rvm/gems/ruby-2.2.2/bin:/Users/ruki/.rvm/gems/ruby-2.2.2@global/bin:/Users/ruki/.rvm/rubies/ruby-2.2.2/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/Users/ruki/.rvm/bin
> os.exec("sudo -E xmake l --root -c 'print(os.getenv(\"PATH\"))'")
/tmp/test:/Users/ruki/.cargo/bin:/Users/ruki/.rvm/gems/ruby-2.2.2/bin:/Users/ruki/.rvm/gems/ruby-2.2.2@global/bin:/Users/ruki/.rvm/rubies/ruby-2.2.2/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/Users/ruki/.rvm/bin
> os.sudo(os.exec, "xmake l --root -c 'print(os.getenv(\"PATH\"))'")
/tmp/test:/Users/ruki/.cargo/bin:/Users/ruki/.rvm/gems/ruby-2.2.2/bin:/Users/ruki/.rvm/gems/ruby-2.2.2@global/bin:/Users/ruki/.rvm/rubies/ruby-2.2.2/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/Users/ruki/.rvm/bin
> os.exec("sudo -E sh -c 'echo $PATH'")
/tmp/test:/Users/ruki/.cargo/bin:/Users/ruki/.rvm/gems/ruby-2.2.2/bin:/Users/ruki/.rvm/gems/ruby-2.2.2@global/bin:/Users/ruki/.rvm/rubies/ruby-2.2.2/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/Users/ruki/.rvm/bin
> os.sudo(os.exec,"sh -c 'echo $PATH'")
/tmp/test:/Users/ruki/.cargo/bin:/Users/ruki/.rvm/gems/ruby-2.2.2/bin:/Users/ruki/.rvm/gems/ruby-2.2.2@global/bin:/Users/ruki/.rvm/rubies/ruby-2.2.2/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/Users/ruki/.rvm/bin

@ghost
Copy link
Author

ghost commented May 24, 2017

I found some info on stackoverflow. Seems ubuntu's behavior on sudo is different from others 😓

As travis-ci uses ubuntu, we have no way to drop $(xmake)

@waruqi
Copy link
Member

waruqi commented May 24, 2017

@titansnow I am going to try adding detect.tool.find_sudo and privilege.sudo extension modules in xmake/modules dir to handle these differences

@waruqi
Copy link
Member

waruqi commented May 24, 2017

@titansnow I have removed the $(xmake) variable and and even if sudo cannot inherit the parent environment, the path of xmake is always in the $PATH because I'll add it's directory to $PATH in the main entry function.

function main.init()

    -- add the directory of the program file (xmake) to $PATH environment
    local programfile = os.programfile()
    if programfile and os.isfile(programfile) then
        os.addenv("PATH", path.directory(programfile))
    else
        os.addenv("PATH", os.programdir())
    end

@ghost
Copy link
Author

ghost commented May 24, 2017

So how to deal with build error on travis-ci? The trouble is sudo cannot find xmake not xmake cannot find itself

@waruqi
Copy link
Member

waruqi commented May 24, 2017

@titansnow Can you give me your failed travis-ci links?

@ghost
Copy link
Author

ghost commented May 24, 2017

Below ⬇️ 😃

@waruqi
Copy link
Member

waruqi commented May 24, 2017

@titansnow

So how to deal with build error on travis-ci?

This seems like another problem about luacov.runner. I will look at it.

The trouble is sudo cannot find xmake not xmake cannot find itself

I think xmake can find itself from os.programfile() and I see travis has passed

@ghost
Copy link
Author

ghost commented May 24, 2017

I think xmake can find itself from os.programfile() and I see travis has passed

That is because 962b99c 😓

@waruqi
Copy link
Member

waruqi commented May 24, 2017

@titansnow Ok, I will look at it.

@waruqi
Copy link
Member

waruqi commented May 24, 2017

@titansnow I'm doing some tests on travis-ci

The path of xmake already exists when running sudo sh -c "echo $PATH". => /home/travis/bin

But xmake still not found,I don't know why. 😢

$ which xmake
/home/travis/bin/xmake

$ sudo sh -c "echo $PATH"
/home/travis/bin:/home/travis/.local/bin:/home/travis/.pyenv/shims:/home/travis/.phpenv/shims:/home/travis/gopath/bin:/home/travis/.gimme/versions/go1.7.linux.amd64/bin:/home/travis/.local/bin:/opt/pyenv/bin:/opt/python/2.7.12/bin:/opt/python/3.5.2/bin:/opt/python/2.6.9/bin:/opt/python/3.2.6/bin:/opt/python/3.3.6/bin:/opt/python/3.4.4/bin:/opt/python/pypy-5.4.1/bin:/opt/python/pypy3-2.4.0/bin:/usr/local/phantomjs/bin:/usr/local/phantomjs:/home/travis/perl5/perlbrew/bin:/home/travis/.nvm/versions/node/v4.1.2/bin:/usr/local/maven-3.1.1/bin:/home/travis/.kiex/elixirs/elixir-1.0.4/bin:/home/travis/.kiex/bin:/usr/local/gradle/bin:/usr/local/clang-3.5.0/bin:/home/travis/.rvm/gems/ruby-2.3.1/bin:/home/travis/.rvm/gems/ruby-2.3.1@global/bin:/home/travis/.rvm/rubies/ruby-2.3.1/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/lib/jvm/java-9-oracle/bin:/usr/lib/jvm/java-9-oracle/db/bin:/home/travis/.phpenv/bin:/home/travis/.rvm/bin:/home/travis/.lua:/home/travis/.local/bin:/home/travis/build/tboox/xmake/install/luarocks/bin

$ sudo sh -c "xmake --root lua versioninfo"
sh: 1: xmake: not found

@ghost
Copy link
Author

ghost commented May 24, 2017

@waruqi The problem is funny: double quote will expand the vars inside 😺

$ sudo sh -c 'echo $PATH'   
/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/snap/bin
$ sudo sh -c "echo $PATH"
/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin:/home/ts/.local/bin

@waruqi
Copy link
Member

waruqi commented May 25, 2017

Did you deal with PATH with spaces and quotes? For example PATH=/home/awd/i am space:/home/awd/i'm quote?

I have fixed it.

lua_pushstring(impl -> lua, data);
lua_setglobal(impl -> lua, "_EXECUTABLE_PATH");

if (!ok)
{
// get the directory
while (size-- > 0)
{
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's necessary to write the following code three times.

                while (size-- > 0)
                {
                    if (data[size] == '/')
                    {
                        data[size] = '\0';
                        break;
                    }
                }

Copy link
Member

Choose a reason for hiding this comment

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

I think you can use this implementation in dev to replace it directly. 😸

xmake._PROJECT_DIR = _PROJECT_DIR
xmake._CORE_DIR = _PROGRAM_DIR .. "/core"
xmake._TOOLS_DIR = _PROGRAM_DIR .. "/tools"
xmake._TEMPLATES_DIR = _PROGRAM_DIR .. "/templates"
xmake._PROJECT_FILE = "xmake.lua"
xmake._EXECUTABLE_PATH = _EXECUTABLE_PATH
Copy link
Member

Choose a reason for hiding this comment

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

I think it's enough to use xmake._PROGRAM_FILE = _PROGRAM_FILE, There's no need to add a redundant variable

@ghost
Copy link
Author

ghost commented May 25, 2017

Done

-- run it with administrator permission and preserve parent environment
runner(program .. " PATH=\"" .. os.getenv("PATH") .. "\" " .. cmd, ...)
runner(program .. " env PATH=\"" .. os.getenv("PATH") .. "\" " .. cmd, ...)
Copy link
Member

Choose a reason for hiding this comment

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

The effect that do not add env seems to be the same, what is the difference?

Copy link
Author

Choose a reason for hiding this comment

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

$ ls ~/awd/build
awd
$ sudo PATH=$HOME/awd/build:$PATH awd
sudo: awd: command not found
$ sudo env PATH=$HOME/awd/build:$PATH awd
hello world!

And it makes the build pass

makefile Outdated
@@ -61,15 +61,17 @@ install:
@# install the xmake directory
@cp -r xmake/* $(xmake_dir_install)
@# make the xmake loader
ifneq ($(PLAT),linux)
Copy link
Member

@waruqi waruqi May 25, 2017

Choose a reason for hiding this comment

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

What's this? Although we can get the executable path on linux, macOS and windows now. But maybe we cannot get it successfully on some linux distributions. So I think we still need export XMAKE_PROGRAM_DIR=/xxx

@waruqi waruqi merged commit 0c98817 into xmake-io:dev May 25, 2017
@waruqi
Copy link
Member

waruqi commented May 25, 2017

Great! 😄

@ghost ghost deleted the installtest branch May 25, 2017 05:19
@waruqi
Copy link
Member

waruqi commented May 25, 2017

@titansnow Can you help me to improve get.sh to uninstall xmake? Thanks! 😄

for example:

$ ./scripts/get.sh __uninstall__

waruqi added a commit that referenced this pull request May 25, 2017
add tests for install & uninstall
(cherry picked from commit 0c98817)
@ghost
Copy link
Author

ghost commented May 25, 2017

ok 👌 I'm also going to add a arg to specify install prefix

@ghost
Copy link
Author

ghost commented May 26, 2017

See 8ffb52a

I have impled __uninstall__ and prefix specification

$ scripts/get.sh __uninstall__
$ env prefix=/usr scripts/get.sh

@waruqi
Copy link
Member

waruqi commented May 26, 2017

@titansnow 👍

+    # uninstall
+    makefile=$(remote_get_content https://github.com/tboox/xmake/raw/master/makefile)
+    while which xmake >/dev/null 2>&1

I think we can check makefile in the current directory first to faster uninstalling. for example:

$ cd xmake
$ ./scripts/get.sh __uninstall__

@ghost
Copy link
Author

ghost commented May 26, 2017

But how could we know the makefile found in local is xmake's not other's?

And the size of makefile is tiny so I think it doesn't trouble

@waruqi
Copy link
Member

waruqi commented May 26, 2017

@titansnow Ok. 👌 Can you create a new pr for this commit?

@ghost
Copy link
Author

ghost commented May 26, 2017

Matte, I have a plan of remote rolling pre-build on installation on Windows. That is, the install script get pre-build binary from appveyor rolling build so that no need to build locally to get newest

@waruqi
Copy link
Member

waruqi commented May 26, 2017

Good idea! 👍 But how can you get pre-build binary file from appveyor?

@ghost
Copy link
Author

ghost commented May 26, 2017

You could have a look on that branch

@waruqi
Copy link
Member

waruqi commented May 26, 2017

@titansnow Ok. 👍 😄

@ghost ghost mentioned this pull request May 26, 2017
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.

None yet

2 participants