Skip to content

Rename nanostack configuration (.cfg) files #7780

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

Merged
merged 2 commits into from
Aug 21, 2018
Merged

Conversation

artokin
Copy link
Contributor

@artokin artokin commented Aug 13, 2018

Description

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Feature
[ ] Breaking change

Arto Kinnunen added 2 commits August 13, 2018 18:44
…..7963594

7963594 Merge branch 'release_internal' into release_external
9e31d11 Update apache license to config-files (ARMmbed#1781)
b8f840c Merge branch 'release_internal' into release_external
9495d94 Rename cfg-files to h-files (ARMmbed#1780)

git-subtree-dir: features/nanostack/sal-stack-nanostack
git-subtree-split: 7963594
* commit 'f292d7ad9ab5da89381d7f54de88b46df573c79d':
  Squashed 'features/nanostack/sal-stack-nanostack/' changes from 4a188ea..7963594
@artokin
Copy link
Contributor Author

artokin commented Aug 13, 2018

@SeppoTakalo , @kjbracey-arm , @deepakvenugopal , @mikter would you please review?

@cmonr
Copy link
Contributor

cmonr commented Aug 13, 2018

@artokin Thanks for providing the PR.

However, would you mind updating the commit such that the files that were moved don't appear as newly created files? It's the difference between doing a git mv <file> vs git rm <file>; git commit; git add <file>

Doing git mv preserves the file history.

@cmonr
Copy link
Contributor

cmonr commented Aug 13, 2018

Could you also update the commit description, since the commits indicate more changes than just those to fix the issue?

@artokin
Copy link
Contributor Author

artokin commented Aug 13, 2018

@cmonr , all the files were changed similary using git mv <file>, no new files were created. Could it be so that github tool shows some file as new file instead of rename?

@cmonr
Copy link
Contributor

cmonr commented Aug 13, 2018

...huh. Interesting. I wonder if the squash commit could have something to do with it.

@geky
Copy link
Contributor

geky commented Aug 13, 2018

It looks like, besides merges, there's only two commits on the subtree. Can you cherry-pick those commits onto a new branch based on master? This is required to make it into a patch release. Future subtree merges will not have any conflicts if the cherry-picked changes match.

@cmonr
Copy link
Contributor

cmonr commented Aug 13, 2018

@artokin Conversely, if you don't mind on having this fix wait until 5.10, we can simply move this PR forward asis.

@SeppoTakalo
Copy link
Contributor

@cmonr This change is applied here as a git subtree pull --squash as we have always done with Nanostack changes. We don't want to apply local changes, or cherry-picks into the Git subtree within Mbed OS repository, it might complicate things later on.

From Mbed OS release point of view, it needs to be threaded as a merge commit. Directly applying it as a patch won't work. Anna has been able to apply these changes to patch release as well, so I assume that it works.

And I would prefer if this goes out into the patch release, as the export builds are broken currently.

@kjbracey
Copy link
Contributor

git mv is just a convenience helper and equivalent to removing then adding - Git doesn't actually store any tracking information for moves and copies, it's always deduced by the log viewer. The addition of the licenses makes some of the differences too big to deduce the move in the squashed result - it would have been clearer in the internal repo.

I thought Anna had had problems cherry-picking these subtree merge PRs in the past, although I reckon it should in theory possible by cherry-picking just the merge commit with the -m option.

But as this follows the already-merged 5.10 release PR won't the 5.9 backport likely conflict anyway? So maybe it would be best to make a separate PR targeting the 5.9 branch.

@adbridge
Copy link
Contributor

As @kjbracey-arm noted, if this is based on top of changes earmarked for 5.10 then yes we are likely to get merge conflicts, so a separate PR would be best. However I don't like this kind of thing happening in general... Should only be for urgent fixes.

Copy link
Contributor

@kjbracey kjbracey left a comment

Choose a reason for hiding this comment

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

I think this is okay to go now

@cmonr
Copy link
Contributor

cmonr commented Aug 17, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Aug 17, 2018

Build : SUCCESS

Build number : 2832
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7780/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci
Copy link

mbed-ci commented Aug 18, 2018

@mbed-ci
Copy link

mbed-ci commented Aug 18, 2018

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 20, 2018

And I would prefer if this goes out into the patch release, as the export builds are broken currently.

Me as well, will review the release label during today's gatekeeping

@kjbracey
Copy link
Contributor

As this PR is built on previous full release and is all subtree-y, this PR is not pickable to 5.9.6.

But fear not, there's already a 5.9-rebased easily-pickable version here: #7789.

@NirSonnenschein
Copy link
Contributor

/morph uvisor-test

@cmonr cmonr merged commit b53a9ea into ARMmbed:master Aug 21, 2018
@artokin artokin deleted the nsrc510_p1 branch August 22, 2018 06:20
pan- pushed a commit to pan-/mbed that referenced this pull request Aug 22, 2018
Rename nanostack configuration (.cfg) files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LPC1768 cannot build blinky in uVision5 due to nanostack errors
10 participants