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

Fixing source/path and GIT_* issues #801

Merged
merged 18 commits into from
Mar 28, 2016
Merged

Conversation

groutr
Copy link
Contributor

@groutr groutr commented Mar 2, 2016

The overarching goal of this PR is to fix issues with source/path (#783).

I uncovered a subtle issue with the Metadata.get_value() method not behaving as expected (see 3bd490d)

groutr added 5 commits March 1, 2016 19:24
By adding default values to the metadata during parsing, a very subtle bug in m.get_value() that rendered the method near useless.  The default would never be returned if the field was validated during parsing (basically any field that required a certain type).

For example, if m was a Metadata object, m.get_value('source/git_rev', 'master') would never return master, even if 'source/git_rev' was never defined in meta.yaml.  Now, m.get_value will return the default type of a field, if defined, whenever default=None.

Also, when accessing the metadata, never assume any field exists beyond package/name and package/version.
@groutr groutr added type::bug describes erroneous operation, use severity::* to classify the type 3_In_Progress [deprecated] use milestones/project boards enhancement and removed type::bug describes erroneous operation, use severity::* to classify the type labels Mar 2, 2016
@stuarteberg
Copy link
Contributor

BTW, #803 addresses some related (but different) issues to the ones you're addressing here.

@groutr
Copy link
Contributor Author

groutr commented Mar 5, 2016

@stuarteberg Could you explain to me what you expect get_git_build_info(src_dir, git_url, None) to mean?

@stuarteberg
Copy link
Contributor

Could you explain to me what you expect get_git_build_info(src_dir, git_url, None) to mean?

I'm thinking about this line:

d.update(**get_git_build_info(d['SRC_DIR'],
                              git_url,
                              m.get_value('source/git_rev')))

If third argument evaluates to None, then get_git_build_info() needs to replace it with a suitable default value. (The original default was 'master', but a better default would be 'HEAD', as proposed in #803.)

@groutr
Copy link
Contributor Author

groutr commented Mar 8, 2016

m.get_value('source/git_rev') won't evaluate to None. By default it will evaluate to text_type()
The only way I can see a None being passed in is if it was explicitly passed in.

@stuarteberg
Copy link
Contributor

By default it will evaluate to text_type()

Ah, okay. Still, for the purposes of this discussion, None and '' have the same meaning.

@groutr
Copy link
Contributor Author

groutr commented Mar 8, 2016

Oh, that third option is supposed to be m.get_value('source/git_rev', 'HEAD'). That's why I fixed the get_value() function.

@stuarteberg
Copy link
Contributor

Sure, that works, too.

@stuarteberg
Copy link
Contributor

BTW, there are likely to be a couple tiny conflicts between this PR and #803. Not sure if you want to merge that first and rebase this or vice-versa.

@groutr
Copy link
Contributor Author

groutr commented Mar 8, 2016

Probably best to merge #803 first.

@stuarteberg
Copy link
Contributor

Also, #790 touches the same function. Might want to look there, too.

@groutr groutr changed the title Fixing git issues Fixing source/path and GIT_* issues Mar 8, 2016
@jakirkham
Copy link
Member

This probably needs its build restarted, but I don't know if it is worth worrying about at this point.

@groutr groutr added 4_Needs_Review [deprecated] use milestones/project boards and removed 3_In_Progress [deprecated] use milestones/project boards labels Mar 25, 2016
@ccordoba12
Copy link

@kalefranz, could you review this one? I'm sure the lack of GIT_* variables is breaking a lot of people's workflows (including ours in Navigator).

@msarahan
Copy link
Contributor

LGTM. Merging. Thanks @groutr @stuarteberg @jakirkham

@stuarteberg
Copy link
Contributor

I like these changes, @groutr. Nicely done. Thanks for merging, @msarahan.

@groutr groutr removed the 4_Needs_Review [deprecated] use milestones/project boards label Apr 8, 2016
@kenodegard kenodegard added type::feature request for a new feature or capability and removed type::enhancement labels Jan 19, 2022
@github-actions github-actions bot added the locked [bot] locked due to inactivity label Jan 20, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked [bot] locked due to inactivity type::feature request for a new feature or capability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants