Make use of system search path, so distributed software is supported too. #449

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
2 participants
@ghost

ghost commented May 28, 2012

No description provided.

Owner

kripken commented May 28, 2012

I don't understand. LLVM_ROOT should be set by the user to the right path, so this should not be needed?

@ghost

ghost commented May 28, 2012

This is not needed - but still possible. Most people (like me) will install the distributed software which installs to some path included in PATH. So why should they have to jump to loops and manually assign the path, when it can be taken from the os.
On the other hand due to the override path in which, if you want to force the locating to use a special path/version, you still can.
And above all it checks for the existance of the binaries so there is no invalid path set.

Owner

kripken commented May 30, 2012

Ok, I see. It does make sense to make this as simple as possible. However this is tricky stuff so we should not land it without tests. Tests should probably go in sanity since they will need to modify ~/.emscripten I assume.

Is the change to scons a separate issue?

Collaborator

ehsan commented Jun 20, 2012

I don't see any scons changes here, perhaps they were part of #448?

Owner

kripken commented Jun 20, 2012

Could be, yeah.

Ok, then what is left is the stuff we need tests for, before merging.

@ghost

ghost commented Sep 15, 2012

Closing as no steps to get this integrated are named.

ghost closed this Sep 15, 2012

Owner

kripken commented Sep 17, 2012

The comment before yours says that we need tests to merge this. Was that not specific enough?

@ghost

ghost commented Sep 18, 2012

What kind of tests? Tests for what?
Ordinarily if all present tests succeed, I would assume that the paths are processed correctly.

Owner

kripken commented Sep 20, 2012

It would be good to actually test the new functionality here, that the system path is checked. But I guess we can include this if nothing is broken in current tests.

Code notes:

  1. The js/html change should be removed from this pull
  2. Use 2 spaces, not 4
  3. Needs a rebase
  4. Does is_exe work on windows? Not sure if executable means the same there
@ghost

ghost commented Sep 20, 2012

  1. It already has it's own pull request.
  2. You probably will have to remind me on this.
  3. I already have a rebased version.
  4. Since I don't own a windows machine I cannot really help with that.

Aside from 4. you did mention no additional test/concerns!?

Owner

kripken commented Sep 20, 2012

  1. Yes, but we might decide we don't want it (see discussion in emscripten-discuss), so we'd need to remove it from here to be able to pull this.
  2. Fair enough, I don't either ;) Was hoping you knew.

As for tests, like I said, I would really like tests for this new functionality. But not mandatory for merging, assuming you see it breaks nothing and the new functionality works for you locally. So let's finish this pull.

Hmm, I did just have one concern just now. Let's show a warning in debug mode (EM_DEBUG variable is true in tools/shared.py) when a system path is used. That way if someone gets odd behavior, and try debug mode, they will see which path is used (so if they have some old version of llvm in their path, they will be told). Or perhaps in debug mode we should print out the paths as they are used, system or no?

kripken reopened this Sep 20, 2012

Owner

kripken commented May 22, 2013

I think we can close this, given the amount of time since last activity?

kripken closed this May 22, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment