Opened 8 years ago

Closed 8 years ago

#1246 closed defect (fixed)

socketcreator breaks "FROM_SOURCE" environment

Reported by: jinmei Owned by: jelte
Priority: medium Milestone: Sprint-20111108
Component: ~Boss of BIND (obsolete) Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: High
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 2 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

socketcreator.Creator() prepends @@LIBEXECDIR@@ to the environment
variable PATH unconditionally:

        self.sockcreator = isc.bind10.sockcreator.Creator("@@LIBEXECDIR@@:" +
                                                          os.environ['PATH'])

It breaks the "FROM_SOURCE" environment because once the socket
creator is invoked installed programs will be spawned instead of the
in-source versions. It specifically breaks run_bind10.sh and system
tests. Especially for the latter this should be fixed ASAP.

Subtickets

Change History (19)

comment:1 Changed 8 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Year 3 Task Backlog

comment:2 Changed 8 years ago by jinmei

  • Milestone changed from Year 3 Task Backlog to Next-Sprint-Proposed
  • Priority changed from major to critical

comment:3 Changed 8 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20111025

comment:4 Changed 8 years ago by jinmei

  • Priority changed from critical to major

comment:5 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 2

comment:6 Changed 8 years ago by jelte

  • Owner set to jelte
  • Status changed from new to assigned

comment:7 Changed 8 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from assigned to reviewing

I've done one extra thing, apart from adding the B10_FROM_SOURCE check; I've also made the code do the path changes on a (deep) copy of the environment, so if they do something bad, at least it won't leak into the rest of the system.

(btw, i also noticed there was some end-of-line whitespace, but i did not fix those at this point because i expect #213 would create lots of conflicts then)

comment:8 Changed 8 years ago by jinmei

  • Owner changed from UnAssigned to jinmei
  • Status changed from reviewing to accepted

comment:9 Changed 8 years ago by jinmei

  • Status changed from accepted to reviewing

comment:10 follow-up: Changed 8 years ago by jinmei

It basically looks okay. Some minor comments:

  • If possible I'd avoid embedding the hardcoded check for B10_FROM_SOURCE in bind10_src.py or at least unify the point of this check (e.g. via a command line option).
  • isn't this test self-contained?
        def test_unchanged_environment(self):
            # Check whether the environment has not been changed
            self.assertEqual(original_os_environ, os.environ)
    
    i.e., it seems to assume that other tests could modify os.environ without the change introduced in this branch. I'd make sure this test itself performs start_all_process() or something.
  • do we need a changelog entry? This is certainly a visible bug, but on the other hand only core developers would care, so mostly irrelevant to end users. I'd leave it to you.

comment:11 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:12 in reply to: ↑ 10 ; follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to jinmei

Replying to jinmei:

It basically looks okay. Some minor comments:

  • If possible I'd avoid embedding the hardcoded check for B10_FROM_SOURCE in bind10_src.py or at least unify the point of this check (e.g. via a command line option).

I did not make it a command-line option (yet, we can consider this,
but it could be part of a different approach to supporting 'in-source'
running)

However, I did unify it in the sense that it'll now set a global
variable in the 'big' check right at the start (so at least the
environment variable name is only used once). Updated the several
checks for the environment variable to use that one.

  • isn't this test self-contained?
        def test_unchanged_environment(self):
            # Check whether the environment has not been changed
            self.assertEqual(original_os_environ, os.environ)
    
    i.e., it seems to assume that other tests could modify os.environ without the change introduced in this branch. I'd make sure this test itself performs start_all_process() or something.

since this test is last, if they actually change the environment, it
would show (note that both the deepcopy and the environment check fix
this, so to see the test failing you'd need to revert all changes).

But if the order isn't guaranteed, or if we feel this is relying too
much on unittest framework implementation details, we could also make
this a helper function and add it to the end of all tests. Would you
prefer that?

  • do we need a changelog entry? This is certainly a visible bug, but on the other hand only core developers would care, so mostly irrelevant to end users. I'd leave it to you.

I'll ask jeremy. In the case we do, I propose this one:

  1. [defect] jelte

The main bind10 script now no longer runs processes from an installed
version when it is run from the source tree with run_bind10.sh. It
will correctly use the source tree build.

comment:13 in reply to: ↑ 12 ; follow-up: Changed 8 years ago by jinmei

Replying to jelte:

  • If possible I'd avoid embedding the hardcoded check for B10_FROM_SOURCE in bind10_src.py or at least unify the point of this check (e.g. via a command line option).

I did not make it a command-line option (yet, we can consider this,
but it could be part of a different approach to supporting 'in-source'
running)

However, I did unify it in the sense that it'll now set a global
variable in the 'big' check right at the start (so at least the
environment variable name is only used once). Updated the several
checks for the environment variable to use that one.

Okay, but the LD PATH hack is expected to be a very short term workaround,
and when it's resolved the "AND_LD" part in ADD_LIBEXEC_AND_LD_PATH will
be meaningless. I'd rename it to something a bit generic, e.g.
ADD_INSTALLED_PATHS. Or we might simply say ADD_LIBEXEC_PATH and
add a comment to the "LD" part that we abuse this variable name for a
short term hack.

  • isn't this test self-contained?
        def test_unchanged_environment(self):
            # Check whether the environment has not been changed
            self.assertEqual(original_os_environ, os.environ)
    
    i.e., it seems to assume that other tests could modify os.environ without the change introduced in this branch. I'd make sure this test itself performs start_all_process() or something.

since this test is last, if they actually change the environment, it
would show (note that both the deepcopy and the environment check fix
this, so to see the test failing you'd need to revert all changes).

But if the order isn't guaranteed, or if we feel this is relying too
much on unittest framework implementation details, we could also make
this a helper function and add it to the end of all tests. Would you
prefer that?

I don't know if the order is guaranteed or not by the unittest module
specification, but I'd generally avoid relyin on test orders. We might
want to disable other part of the tests or we might want to reorder
the tests, and I'd like to make it less susceptible to such events.

  • do we need a changelog entry? This is certainly a visible bug, but on the other hand only core developers would care, so mostly irrelevant to end users. I'd leave it to you.

I'll ask jeremy. In the case we do, I propose this one:

  1. [defect] jelte

The main bind10 script now no longer runs processes from an installed
version when it is run from the source tree with run_bind10.sh. It
will correctly use the source tree build.

This looks okay. I'd leave it to you and Jeremy whether to actually
include it.

comment:14 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:15 in reply to: ↑ 13 ; follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to jinmei

Replying to jinmei:

Replying to jelte:

However, I did unify it in the sense that it'll now set a global
variable in the 'big' check right at the start (so at least the
environment variable name is only used once). Updated the several
checks for the environment variable to use that one.

Okay, but the LD PATH hack is expected to be a very short term workaround,
and when it's resolved the "AND_LD" part in ADD_LIBEXEC_AND_LD_PATH will
be meaningless. I'd rename it to something a bit generic, e.g.
ADD_INSTALLED_PATHS. Or we might simply say ADD_LIBEXEC_PATH and
add a comment to the "LD" part that we abuse this variable name for a
short term hack.

haha, i had originally made it ADD_LIBEXEC_PATH but then ran into the
LD code :p

Changed it to ADD_LIBEXEC_PATH and comment.

since this test is last, if they actually change the environment, it
would show (note that both the deepcopy and the environment check fix
this, so to see the test failing you'd need to revert all changes).

But if the order isn't guaranteed, or if we feel this is relying too
much on unittest framework implementation details, we could also make
this a helper function and add it to the end of all tests. Would you
prefer that?

I don't know if the order is guaranteed or not by the unittest module
specification, but I'd generally avoid relyin on test orders. We might
want to disable other part of the tests or we might want to reorder
the tests, and I'd like to make it less susceptible to such events.

ok, added a call to it to the end of the check_started methods (so it
will be called in every test that checks whether things have or have
not started)

comment:16 Changed 8 years ago by jelte

  • Milestone changed from Sprint-20111025 to Sprint-20111108

comment:17 in reply to: ↑ 15 Changed 8 years ago by jinmei

Looks okay.

comment:18 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:19 Changed 8 years ago by jelte

  • Resolution set to fixed
  • Status changed from reviewing to closed

Thank you, merged, closing

Note: See TracTickets for help on using tickets.