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

More segment improvements #158

Merged
merged 3 commits into from
Jul 4, 2023
Merged

More segment improvements #158

merged 3 commits into from
Jul 4, 2023

Conversation

csweichel
Copy link
Collaborator

Description

Report build start and finish. Let's us detect build runner failures.

@codecov
Copy link

codecov bot commented Jul 4, 2023

Codecov Report

Merging #158 (346fddb) into main (53be4e9) will decrease coverage by 0.03%.
The diff coverage is 0.00%.

❗ Current head 346fddb differs from pull request most recent head 3ab6432. Consider uploading reports for the commit 3ab6432 to get more accurate results

@@           Coverage Diff            @@
##            main    #158      +/-   ##
========================================
- Coverage   8.39%   8.37%   -0.03%     
========================================
  Files         20      20              
  Lines       3394    3403       +9     
========================================
  Hits         285     285              
- Misses      3057    3066       +9     
  Partials      52      52              

cmd/exec.go Outdated
@@ -213,6 +213,7 @@ func executeCommandInLocations(execCmd []string, locs []commandExecLocation, par
go io.Copy(ptmx, os.Stdin)
if parallel {
wg.Add(1)
loc := loc
Copy link
Member

Choose a reason for hiding this comment

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

What's the intention here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

capture the variable because it's used in the go routine below

Copy link
Member

@aledbf aledbf Jul 4, 2023

Choose a reason for hiding this comment

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

Right, why not pass the variable to the goroutine as a parameter to make it explicit?

Like

go func(loc XXX) {
		defer wg.Done()

		err = cmd.Wait()
		if err != nil {
			log.Errorf("execution failed in %s (%s): %v", loc.Name, loc.Dir, err)
		}
}(loc)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@csweichel csweichel merged commit aa031b1 into main Jul 4, 2023
6 checks passed
@csweichel csweichel deleted the cw/more-segment-improvements branch July 4, 2023 09:08
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.

2 participants